-
Notifications
You must be signed in to change notification settings - Fork 2
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
implement /get-friends route #151
Conversation
is this ready for review? |
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.
Looks good. I also implemented a simple get-friends
route, where it returns the email of all friends. I think your approach is better.
@@ -14,9 +14,10 @@ def token_response(token: str): | |||
"access_token": token | |||
} | |||
|
|||
def signJWT(user_id: str) -> Dict[str, str]: | |||
def signJWT(user_id: str, email: str) -> Dict[str, str]: |
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.
I think you don't have to include email in JWT token, but it's also fine if you do.
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.
I agree -- I think UUID is the more important one. Currently, the JWT only stores the email, so this comment is a little misleading. I am moving the email to a separate parameter and adding UUID as the new first parameter as we care about that one more. Should we get rid of email in it's entirety?
friends = [] | ||
for friend_email in user["friends"]: | ||
fu = db.get_user_by_email(friend_email) | ||
friends.append(json.loads(json_util.dumps(fu))) |
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.
We can optimize this later by having a single batch query. For now, this is good.
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.
Do you know the method off the top of your head? I forgot we could do that. I can do some research tomorrow otherwise
yes, I'll add the label, thanks :D |
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.
Looks good!
No description provided.