Skip to content
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

Added ability for username to act as identifier for API user detail endpoint in addition to primary key (/api/users/<userID> OR /api/users/<username>) #321

Merged
merged 9 commits into from
Dec 4, 2023

Conversation

abrahmasandra
Copy link
Contributor

Goal: In this PR, we allow users of the API to access the Detail view of a given User object using the user's username as a slug instead of always having to use the user's id. For a user of the API to get a given User's ID, they need to go through the entire List view of user's and find the specific User they are looking for and then look at the id of that User.

The username on the other hand, is a unique identifier for a User object, that is human readable and can be shared/accessed easily. This way, it is easier for someone that uses the API to get details of a specific user, as one does not need to know it's primary key in the model.

We will support: /api/users/<user_id> AND /api/users/<username> to GET/PATCH/DELETE the Detail view of the User with the given id or username

Copy link
Contributor

@cuyakonwu cuyakonwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see me comment on conflicting query options

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a user goes by the name "1234"? That could cause an issue with searching for the correct user. A better implementation could have you search through user name first and store usernames as "User#user_id" That way you can query id numbers by using the #, and we can exclude # from potential username characters.

Copy link
Contributor Author

@abrahmasandra abrahmasandra Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made a commit that validates the User's username, and ensures that it cannot be fully numeric. This way, we will not have this problem, as a given URL parameter can only be interpreted as either a username OR an id but not BOTH.

NOTE: This commit is in PR #291, because in that PR, I created the username field for the User model, so it is only consistent that I put restrictions on the username field in that PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You must reach out to the users team before I will approve any changes to the User model.

Copy link
Contributor

@cuyakonwu cuyakonwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

Copy link
Contributor

@majorsylvie majorsylvie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per PR #291 , this PR involves changes that only fully work once the restriction on fully numeric usernames is merged to dev.

Until pr #291 is fully merged I will not merge this.

def get_object(self):
lookup_value = self.kwargs.get(self.lookup_field)

# If the lookup_value is an integer, use the id field
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good code comment!

@majorsylvie majorsylvie self-requested a review December 4, 2023 21:13
@majorsylvie majorsylvie reopened this Dec 4, 2023
Copy link
Contributor

@majorsylvie majorsylvie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works as expected!

I can POST users with unique usernames/email while having overlapping names, then go to api/users/<username> and get their detail view.

While still being able to access api/users/<pk>

Great work and keep moving forward! We're almost done!

@majorsylvie majorsylvie merged commit eb2f8e2 into dev Dec 4, 2023
2 checks passed
@majorsylvie majorsylvie deleted the apis/user-detail-slugs branch December 4, 2023 21:42
@majorsylvie
Copy link
Contributor

Issue Score: Excellent

Comments:
Great job y'all!
Appreciate the back and forth on review as well as linking relevant issues and problems as they come up.

@abrahmasandra
Copy link
Contributor Author

Closing Comment: Successfully, completed the slug functionality to get the detail view of a User. Can now access the detail view going to endpoint: /api/users/<user_id> OR /api/users/<username>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
3 participants