-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix get_policy_search
endpoint to return 404
when no policy is found
#1854
Fix get_policy_search
endpoint to return 404
when no policy is found
#1854
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tawandamoyo, thanks for your work on this. I had a couple of recommended changes, but looking forward to merging this down the road.
policyengine_api/endpoints/policy.py
Outdated
message=f"Database query failed for {country_id} with query '{query}", | ||
) | ||
return Response( | ||
json.dumps(body), status=500, mimetype="application/json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json.dumps(body), status=500, mimetype="application/json" | |
json.dumps(body), status=404, mimetype="application/json" |
See above on why I say 404 is better here than 500
7c05cd9
to
3010ed0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tawandamoyo, thanks for your adjustments to this. One last thing - I think we should send the error message as a part of the response, and I've added the typical method of doing so as a suggestion. After that, this will be good to go.
policyengine_api/endpoints/policy.py
Outdated
return Response( | ||
json.dumps(body), status=200, mimetype="application/json" | ||
) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except: | |
except Exception as e: |
One minor thing - we can actually return the error message with the response, so I think that'd be for the best.
3010ed0
to
22fb8c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes @tawandamoyo! It'll be great to have these.
Fixes #416
Changes
get_policy_search