-
Notifications
You must be signed in to change notification settings - Fork 5
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
add random avatar generation to backend #327
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.
Left a few more comments! I think we'll need to have a discussion later about whether this is the best solution for generating images, but for now I've focused on the rest of the PR which is look good!
One semi-major comment: can we move this logic to be run as a signal when the User is created? (see signals.py
in the user folder for examples)
Okay, regarding the image generation, I think an approach in this style would be better: https://stackoverflow.com/a/30669765, where we just calculate the rgb value for each pixel in the image. That answer is for a circular gradient: you can adapt it for whatever you think is the most appropriate choice (the frontend previously did a 45-degree linear gradient between two colors), but hopefully this solution is more concise and doesn't need numpy. |
Actually, having the logic here has the advantage that an avatar will be retroactively generated for users without avatars, which is kind of nice. I'm fine with keeping it in the |
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.
This is looking great! But there are a couple tricky issues we need to sort out, probably with discussion with @j-mao:
- the meaning of
has_avatar
andavatar_uuid
and how we want to manage them (e.g. should the UUID be nullable and shouldhas_avatar
becomehas_uploaded_avatar
?) - we also need the same thing for teams. How we can handle the same thing there with minimal repeated code? (And ideally address Clean up repeated avatar logic #302 along the way)
Based on discussion with @j-mao: let's abstract away the random avatar generation process into a separate function that is called from |
Is this ready for review yet? |
return avatar | ||
|
||
|
||
def get_avatar_url(entity): |
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.
The main thing we should definitely do is abstract away the random avatar generation code (the generate_avatar
function); you've tried to abstract get_avatar_url
as well, but this seems a bit brittle (e.g. see the comment about hardcoding entities). I would say the simplest solution would be to move the get_avatar_url
logic back to user and team (copying a bit of logic, which is ok with me for now) and then only keep the generate_avatar
function here.
If we really wanted to abstract this logic, it would probably make the most sense to have a custom avatar field with a get_avatar_url
member function. But that is a bit messy since it changes e.g. the DB model, so probably out of scope for this PR. Pinging @j-mao for any thoughts
|
||
if not entity.has_avatar: | ||
|
||
entity.has_avatar = True |
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.
so has_avatar
indicates "has uploaded avatar". Which should be true only if a user has uploaded an avatar, not if they have a randomly generated one. So I think has_avatar
should be kept False
until a user actually uploads an avatar. (Perhaps it could be renamed has_uploaded_avatar
...)
Instead, for randomly generating avatars, we should have avatar_uuid
be null by default and set it to a value once a randomly generated avatar has been created (for existing users in prod, we would need to do a DB update to set avatar_uuid
to null if has_avatar
is False). This is a bit low priority, so I would suggest prioritizing #370 first.
Is this ready for another review yet? |
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.
Noting here that this needs to be rebased after #390 is merged, because the migration needs to be re-generated.
Closing b/c we'll display a default profile picture instead |
Moved the random avatar generation to the backend
Closes #238