search

Police-Data-Accessibility-Project / data-sources-app

An API and UI for using and maintaining the Data Sources database
MIT License
4 stars 5 forks source link

Create `/user/{user_id}` `GET` endpoint #508

Open maxachis opened 2 weeks ago

maxachis commented 2 weeks ago

Context

Currently, we have no means of getting various properties of a user from the API. For example:

Requirements

Tests

Docs

Open questions

josh-chamberlain commented 2 weeks ago

@maxachis I edited the requirements above to mention the user profile; the GET by user_id need not include all those things, but that's the main way we'd use the endpoint.

yes, let's include the permissions! seems useful. we may even want to display those on the user profile—best to air those things out rather than have them take effect invisibly, imo. i'll edit the issue

maxachis commented 2 weeks ago

@josh-chamberlain @joshuagraber I'd also like to clarify a few things about the /user endpoints, as it's possible we may need some reorganization of the endpoints.

Currently, we have the following endpoints for /user

This means /user/{user_id} would cause confusion/conflicts with /data-requests and /recent-searches.

One solution to this is simply /user/by-id/{user_id}. That being said, I feel like there's some other issues that might or might not be a problem:

So things don't seem fully consistent. How, then, to address?

joshuagraber commented 2 weeks ago

Yeah @maxachis I agree this is inconsistent. Thank you for bringing it up.

This means /user/{user_id} would cause confusion/conflicts with /data-requests and /recent-searches.

Is there a way in Flask to define certain routes, in this case /user/data-requests and /user/recent-searches, and then have anything else on that route path become a param if it doesn't match any of the defined routes?

In that case, you could have cake and eat it too.

Another option would be to just return the user's data requests and recent searches (and any other necessary data) as a part of a single user object returned by GETto /user. Still could look up by JWT, so GET is viable. In that case, POST can create, PUT to /user/:id can update, and it's all very clean and tidy. We'd remove the conflict issue entirely. What do you think about that?

maxachis commented 2 weeks ago

Is there a way in Flask to define certain routes, in this case /user/data-requests and /user/recent-searches, and then have anything else on that route path become a param if it doesn't match any of the defined routes?

In that case, you could have cake and eat it too.

This is viable! I will look into this.

maxachis commented 2 weeks ago

@josh-chamberlain @joshuagraber Working on this, how should permissions work for getting information on users?

  1. I can keep it simple and just say only Admins can get users by id.
  2. I can have it so that only Admins and the user themselves can get a user by id
  3. I can have it so that, depending on the permissions available, different amounts of user information are displayed.
joshuagraber commented 1 week ago

I'd say 2 is probably the move @maxachis, but I could see 3 being a good option as well.

josh-chamberlain commented 1 week ago

@maxachis I typed a response but didn't hit enter... number 2 covers it!

maxachis commented 1 week ago

Another option would be to just return the user's data requests and recent searches (and any other necessary data) as a part of a single user object returned by GETto /user. Still could look up by JWT, so GET is viable. In that case, POST can create, PUT to /user/:id can update, and it's all very clean and tidy. We'd remove the conflict issue entirely. What do you think about that?

@joshuagraber So let's visit this, because in addition to the issue with overlap, there's some other stuff to take into account.

@josh-chamberlain indicated to me in off-Hub conversation that the minimum viable product (MVP) for this endpoint is providing anything that is not already accounted for by other endpoints in generating the user profile. For those following along at home, that includes:

So as an MVP, this would thus provide:

BUT I also know that @joshuagraber prefers minimizing endpoint calls, and as structured, this would involve 5 separate endpoint calls.

The GET endpoint could be consolidated to include data from all of these (although that would still require 5 or more calls to the database). The endpoint could even be modified to allow us to specify only what user information we want.* We could then, if we so desire, eliminate some of those other endpoints, or keep them as a means of separating the frontend logic (even if their backend logic would overlap with the Big Daddy endpoint)

All of these would incur additional development time beyond the MVP. Maybe not a lot, since a lot of this logic has already been developed, but some. But maybe that's worthwhile for consolidating and organizing.

What do you both think?

* Some might call this GraphQL envy, but I disgress.

joshuagraber commented 1 week ago
  • Some might call this GraphQL envy, but I disgress.

Yeah. Isn't GQL nice?

prefers minimizing endpoint calls, and as structured, this would involve 5 separate endpoint calls.

Yeah... There's not much point to fetching a user's data if it doesn't include much of the user's data. I can call 5 separate endpoints here, but that's 5 points of failure as opposed to 1, 5 network requests, yada, yada.

My preference would be one big user object from 1 API call.

I do think it would still be valuable to keep the separate endpoints as well, though.

josh-chamberlain commented 1 week ago

@maxachis ok, I'm happy to be overruled here and 1 big user object is OK by me!