-
Notifications
You must be signed in to change notification settings - Fork 9
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 user avatar to logged_in_user
route and adjusted frontend
#342
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.
Nice - this works great! Some small suggestions below, then this is ready for merge 🚀
from pydantic import BaseModel | ||
# Using the same http client as sumo | ||
import httpx | ||
import base64 |
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.
Move this line to top import group (base64
is std.lib)?
async with httpx.AsyncClient() as client: | ||
result = await client.get("https://graph.microsoft.com/v1.0/me/photo/$value", headers=headers) | ||
if result.status_code == 200: | ||
user_info.avatar_b64str = base64.b64encode(result.content) | ||
|
||
async with httpx.AsyncClient() as client: | ||
result = await client.get("https://graph.microsoft.com/v1.0/me", headers=headers) | ||
if result.status_code == 200: | ||
user_info.display_name = json.loads(result.content.decode("utf-8")).get("displayName", None) |
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 don't know httpx
enough yet to immediately know the answer to this: Are these two calls done sequentially here? If they are, and since they are independent, we could perhaps do them in parallell and just await
until both are done before returning
? Should speed up the logged_in_user
response time in that case.
try: | ||
self.get_graph_access_token() | ||
return True | ||
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 ValueError: |
@@ -28,6 +30,19 @@ def __eq__(self, other: Any) -> bool: | |||
|
|||
def get_username(self) -> str: | |||
return self._username | |||
|
|||
def get_graph_access_token(self) -> str: | |||
if isinstance(self._graph_access_token, str) and len(self._graph_access_token) > 0: |
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.
if isinstance(self._graph_access_token, str) and len(self._graph_access_token) > 0: | |
if isinstance(self._graph_access_token, str) and self._graph_access_token: |
<img | ||
src={`data:image/png;base64,${userInfo.avatar_b64str}`} | ||
alt="Avatar" | ||
className="w-4 h-4 rounded-full" |
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.
Should we make the picture slightly larger, same size as icon?
className="w-4 h-4 rounded-full" | |
className="w-5 h-5 rounded-full" |
Fixes #332.