Endpoint users/ GET method #356
Conversation
|
|
||
|
|
||
| **`GET`** `/api/users/EMAIL` | ||
|
|
There was a problem hiding this comment.
Using the user's email address as the unique identifier for lookup in a URL feels weird (as a login name it's fine). I'd rather see a user id or username here. Not adamantly opposed, just want to discuss it before it's implemented
There was a problem hiding this comment.
Yea, I agree it's weird and think it's worth revisiting. This particular endpoint isn't implemented in this PR.
...I think we'll need to check in on the end-to-end design to decide when/how we fetch just one user's info
There was a problem hiding this comment.
I agree with @erikjasso as I'm not opposed to it, but I believe its common practice to use a user id for this action.
There was a problem hiding this comment.
In our spec, email is the only public-facing identifier for a user because we eliminated the "username" field, deciding it was functionally redundant with just using an email address, and the user_id field is a UUID generated by the database.
I can pull this design question into a new issue since it's not in this PR
There was a problem hiding this comment.
I have seen this done as an /api/users/search as a post where you send the required information along in the request ie: Email. That way you don't have to expose the UUID to the front end.
I agree that it could be beneficial to spec out naming conventions and requisite data for the endpoints at our next meeting.
| @@ -51,5 +51,24 @@ def post(self): | |||
|
|
|||
| return jsonify(response_data), 201 | |||
|
|
|||
| @admin_auth_required | |||
| def get(self): | |||
There was a problem hiding this comment.
Perhaps we can name this index as it is indexing all users? I think the name get is very generic as it could refer to get_users, get_user, get_user_new, or get_user_edit?
There was a problem hiding this comment.
This function is called get because it's the GET http request of the /api/users/ endpoint, and the MethodView class in Flask for defining endpoints needs to use the same name.
I agree it's all a little too generic and needs redesign. I think the present users.GET to get all users seems fine but users.POST to create a new user is confusing. And the GET for a single user needs to be different. This deserves its own issue to hash out a naming scheme.
There was a problem hiding this comment.
I see. Should we write down all of our endpoints (URI's), and document how they're going to be routed so that we can use some naming conventions that imply intent?
|
|
||
| for (email, hashed_password, admin) in [ | ||
| (r['email'], r['hashed_password'], r['admin']) for r in users_get_endpoint_result]: | ||
| self.verify_user_data(email, hashed_password, admin) |
There was a problem hiding this comment.
What is this for loop for after the assert statement?
There was a problem hiding this comment.
It manually checks that the entire returned set of entries from the endpoint match what's in the db.
|
Merging this. this GET endpoint needs to be refactored a bit, but it's fairly minor and this endpoint may be useful for now for working on frontend integration. |
* added users get endpoint, with required new database operation, and tests for both
This is a backend API call to get the list of users currently in the database.
The new code includes the necessary db operations, the endpoint, test cases, and the design spec added to our design.md document.
This endpoint is needed for completing the frontend Users component, issue #91