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

Add dynamic avatar view for project avatars #181

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,11 @@
{% endblock chip_classes %}

{% block chip_icon %}
{% if project.remote_repository %}
<img src="{{ project.remote_repository.avatar_url }}" />
{% else %}
{% comment %}
For now, this is using Dicebear, which has a few random generators for
user/project icons. We don't need to use Dicebear, but could use this or
similar to generate files on disk and use an md5 hash or similar to link
to the same image always.
{% endcomment %}
<img src="https://api.dicebear.com/5.x/shapes/svg?size=64&backgroundColor=b8abd4&shape1Color=8c74c1&shape2Color=7854c5&shape3Color=f0f0ff&seed={{ project.pk }}" />
{% endif %}
<img src="{% url 'theme_avatar_project' project.slug %}" />
{% endblock chip_icon %}

{% block chip_text %}
{{ project.name }}
{{ project.name }}
{% endblock chip_text %}

{% block chip_detail_text %}
Expand All @@ -39,21 +29,11 @@
{% endblock popupcard_image %}

{% block popupcard_header %}
{{ project.name }}
{{ project.name }}
{% endblock popupcard_header %}

{% block popupcard_right %}
{% if project.remote_repository %}
<img class="ui mini right floated rounded image" src="{{ project.remote_repository.avatar_url }}" />
{% else %}
{% comment %}
For now, this is using Dicebear, which has a few random generators for
user/project icons. We don't need to use Dicebear, but could use this or
similar to generate files on disk and use an md5 hash or similar to link
to the same image always.
{% endcomment %}
<img class="ui mini right floated rounded image" src="https://api.dicebear.com/5.x/shapes/svg?size=64&backgroundColor=b8abd4&shape1Color=8c74c1&shape2Color=7854c5&shape3Color=f0f0ff&seed={{ project.pk }}" />
{% endif %}
<img class="ui mini right floated rounded image" src="{% url 'theme_avatar_project' project.slug %}" />
{% endblock popupcard_right %}

{% block popupcard_meta %}
Expand All @@ -63,7 +43,7 @@

{% block popupcard_content %}
<div class="ui small list">

<div class="item">
<i class="fad fa-code-fork icon"></i>
<div class="content">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,4 @@
to the same image always.
{% endcomment %}

{% if project.remote_repository %}
<img class="ui small rounded image" src="{{ project.remote_repository.avatar_url }}" />
{% else %}
<img class="ui small rounded image" src="https://api.dicebear.com/5.x/shapes/svg?size=64&backgroundColor=b8abd4&shape1Color=8c74c1&shape2Color=7854c5&shape3Color=f0f0ff&seed={{ project.pk }}" />
{% endif %}
<img class="ui small rounded image" src="{% url 'theme_avatar_project' project.slug %}" />
25 changes: 25 additions & 0 deletions readthedocsext/theme/templates/theme/images/avatar.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
14 changes: 14 additions & 0 deletions readthedocsext/theme/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# pylint: disable=missing-docstring

from django.urls import re_path
from readthedocs.constants import pattern_opts

from .views import AvatarImageProjectView

urlpatterns = [
re_path(
r"avatar/project/(?P<project_slug>{project_slug})/$".format(**pattern_opts),
AvatarImageProjectView.as_view(),
name="theme_avatar_project",
),
]
108 changes: 108 additions & 0 deletions readthedocsext/theme/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import random

from django.http.response import HttpResponseRedirect
from django.shortcuts import get_object_or_404
from django.views.generic.base import TemplateView
from readthedocs.projects.models import Project


class AvatarImageBaseView(TemplateView):

"""
Base view for project, organization, team or user avatars.
"""

http_method_names = ["get", "head", "options"]

template_name = "theme/images/avatar.svg"
content_type = "image/svg+xml"

# Primary to secondary color gradient, generated using a gradient generator
COLORS = [
"#0993af",
"#0090b7",
"#008bbe",
"#0087c5",
"#0081cb",
"#007bcf",
"#1d73d1",
"#446bd0",
"#6060cc",
"#7854c5",
]

def get(self, request, *args, **kwargs):
self.object = self.get_object()
remote_avatar_url = self.get_remote_avatar_url()
if remote_avatar_url:
return HttpResponseRedirect(remote_avatar_url)
else:
context = self.get_context_data()
return self.render_to_response(context)

def get_object(self):
raise NotImplementedError

def get_queryset(self):
raise NotImplementedError

def get_remote_avatar_url(self):
raise NotImplementedError

def get_avatar_color(self):
random.seed(self.object.pk)
return random.choice(self.COLORS)
Comment on lines +53 to +54
Copy link
Member

Choose a reason for hiding this comment

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

Before I forget, this should make a separate instance https://docs.python.org/3/library/random.html#random.Random, instead of setting the seed globally.


def get_avatar_letters(self):
raise NotImplementedError

def get_context_data(self):
# Truncate letters, use a max of 5 letters for now. More than that and
# text is too tiny.
letters = self.get_avatar_letters()[0:5]
return {
"letters": letters.lower(),
# Font size is proportional to the number of letters
"font_size": 55 - (max(0, len(letters)) * 5),
"background_color": self.get_avatar_color(),
}


class AvatarImageProjectView(AvatarImageBaseView):

"""
Project avatar configuration.

Use the project name or slug for the image letters, and use the project pk
to seed the randomization for background color. A remote URL can be
specified by the attached remote repository, if any.
"""

def get_queryset(self):
return Project.objects.public(self.request.user)

def get_object(self):
queryset = self.get_queryset()
project_slug = self.kwargs.get("project_slug")
return get_object_or_404(
queryset,
slug=project_slug,
)
Comment on lines +81 to +90
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to perform authorization here and also query the database? I'd remove this and do not perform any validation against the DB on the slug received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's doing a db query to get the remote_repo.avatar_url for projects that have one. That is, this view isn't outputting a generated image for every project, only projects without a connected remote repo.

So it seems like authorization would be required too, or a malicious user could scan for project slugs.

The reason for this is because API driven views don't have the same data as template driven views -- namely project remote repo details and project permissions. I would still need to maintain duplicate logic in templates and JS to show the correct avatar, but I think my comment above would at least make this easier.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that the frontend only calls this endpoint when it doesn't have the remote_repo.avatar_url. So, when trying to display the avatar for a repository, if it's not available, it calls this endpoint and the "two letters avatar" is generated -- without authentication. Isn't it that possible?

Copy link
Contributor Author

@agjohnson agjohnson Jul 11, 2023

Choose a reason for hiding this comment

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

That possible, and it's also not hard to do this on the front end side either.

However, the reason I wanted this view was because we don't always have project.remote_repo.avatar_url in API responses, so our front end code can't make the same decision that our backend code can. Consolidating this on the backend solves that, but @stsewd noted security/caching issues.

So, I'm probably leaning towards just doing this on the front end side and saving 10-20 requests per page load. I just need readthedocs/readthedocs.org#10513, which would be a good place to jump in.

Copy link
Member

Choose a reason for hiding this comment

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

However, the reason I wanted this view was because we don't always have project.remote_repo.avatar_url in API responses

Can we make our API to return this attribute instead to simplify this work and also allow CDN caching without leaking anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should have clarified. readthedocs/readthedocs.org#10513 would allow me to use the v3 API, which has project expansion and would therefore would give me the avatar_url.

We could make the call whether to output the SVG on the frontend or backend, but I'd still probably lean towards a frontend implementation as the SVG that is generated is only ~600B. The request time for all of those images would probably be less than parameterizing the SVG on the frontend side.

But caching an endpoint like you're describing would be an option still.

Copy link
Member

Choose a reason for hiding this comment

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

If we can easily do it in the frontend side, I'd prefer that 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, I'll try this implementation again when I'm back and we can compare the load timing, etc. I think the front end implementation is looking most likely though.


def get_remote_avatar_url(self):
try:
return self.object.remote_repository.avatar_url
except (AttributeError, ValueError):
return

def get_avatar_letters(self):
# Try using the project name first, as the slug could have an organization slug
# prepended to the project slug (this leads to redundant acronyms).
words = self.object.name.split(" ")
# However, some project names are just something machine readable
# anyways. See if the slug produces a longer acronym
slug_words = self.object.slug.split("-")
if len(slug_words) > len(words):
words = slug_words

return "".join([word[0:1] for word in words])