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.

Sign up for GitHub

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

Jump to bottom

Serve badges directly from local filesystem #4561

Merged
merged 5 commits into from
Aug 27, 2018
Merged

Serve badges directly from local filesystem #4561

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e99a1f
Serve badges directly from local filesystem
davidfischer Aug 23, 2018
53376a2
Remove call to static
davidfischer Aug 23, 2018
1f39313
Add a content type test
davidfischer Aug 23, 2018
0e19c18
Autolint fix and small performance optimization
davidfischer Aug 27, 2018
2b3c3b7
Log an error when the filesystem can't be read
davidfischer Aug 27, 2018
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
48 changes: 29 additions & 19 deletions readthedocs/projects/views/public.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
from django.conf import settings
from django.contrib import messages
from django.contrib.auth.models import User
from django.contrib.staticfiles.templatetags.staticfiles import static
from django.core.cache import cache
from django.core.urlresolvers import reverse
from django.http import Http404, HttpResponse, HttpResponseRedirect
Expand Down Expand Up @@ -115,25 +114,36 @@ def project_badge(request, project_slug):
style = request.GET.get('style', 'flat')
if style not in ("flat", "plastic", "flat-square", "for-the-badge", "social"):
style = "flat"
badge_path = 'projects/badges/%s-' + style + '.svg'

# Get the local path to the badge files
badge_path = os.path.join(
os.path.dirname(__file__),
'..',
'static',
'projects',
'badges',
'%s-' + style + '.svg',
)

version_slug = request.GET.get('version', LATEST)
try:
version = Version.objects.public(request.user).get(
project__slug=project_slug, slug=version_slug)
except Version.DoesNotExist:
url = static(badge_path % 'unknown')
return HttpResponseRedirect(url)
version_builds = version.builds.filter(type='html',
state='finished').order_by('-date')
if not version_builds.exists():
url = static(badge_path % 'unknown')
return HttpResponseRedirect(url)
last_build = version_builds[0]
if last_build.success:
url = static(badge_path % 'passing')
else:
url = static(badge_path % 'failing')
return HttpResponseRedirect(url)
file_path = badge_path % 'unknown'

version = Version.objects.public(request.user).filter(
project__slug=project_slug, slug=version_slug).first()

if version:
last_build = version.builds.filter(type='html', state='finished').order_by('-date').first()
if last_build:
if last_build.success:
file_path = badge_path % 'passing'
else:
file_path = badge_path % 'failing'

with open(file_path) as fd:
Copy link
Member

Choose a reason for hiding this comment

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

Does it worth here to handle any possible exception when reading the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that is necessary. If Python can't read a local file, we have big problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe protect with ioerror/oserror catch here? Logic can either return unknown image or at least a 4xx response instead of 5xx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a 5xx response is correct. Maybe a 503 instead of a 500, but definitely not a 4xx. This is a server error and not a client error.

If we can't read from the local filesystem, we can't return the unknown image except with a redirect. How about a 503 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do wonder whether this is even necessary though. We don't protect against failing to read the local filesystem for Django templates. Is this really any different.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah true, i was considering a 4xx response from the perspective of nginx -- if the permissions were wrong or the files went away, nginx would return the respective 4xx response. agree that 5xx is maybe better for this though.

I think my only concern is to log something helpful, and perhaps avoid our default 5xx page if it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do a 503 with some logging. That sounds like a plan.

return HttpResponse(
fd.read(),
content_type='image/svg+xml',
)


def project_downloads(request, project_slug):
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/rtd_tests/tests/test_privacy_urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ class PublicProjectMixin(ProjectMixin):
response_data = {
# Public
'/projects/pip/downloads/pdf/latest/': {'status_code': 302},
'/projects/pip/badge/': {'status_code': 302},
'/projects/pip/badge/': {'status_code': 200},
}

def test_public_urls(self):
Expand Down
37 changes: 19 additions & 18 deletions readthedocs/rtd_tests/tests/test_project_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@
from mock import patch
from django.test import TestCase
from django.contrib.auth.models import User
from django.contrib.staticfiles.templatetags.staticfiles import static
from django.contrib.messages import constants as message_const
from django.core.urlresolvers import reverse
from django.http.response import HttpResponseRedirect
from django.views.generic.base import ContextMixin
from django_dynamic_fixture import get
from django_dynamic_fixture import new
from django_dynamic_fixture import get, new

import six

Expand Down Expand Up @@ -424,10 +422,6 @@ def get_project_queryset(self):
class TestBadges(TestCase):
"""Test a static badge asset is served for each build."""

# To set `flat` as default style as done in code.
def get_badge_path(self, version, style='flat'):
return static(self.BADGE_PATH % (version, style))

def setUp(self):
self.BADGE_PATH = 'projects/badges/%s-%s.svg'
self.project = get(Project, slug='badgey')
Expand All @@ -436,32 +430,39 @@ def setUp(self):

def test_unknown_badge(self):
res = self.client.get(self.badge_url, {'version': self.version.slug})
static_badge = self.get_badge_path('unknown')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'unknown')

# Unknown project
unknown_project_url = reverse('project_badge', args=['fake-project'])
res = self.client.get(unknown_project_url, {'version': 'latest'})
self.assertContains(res, 'unknown')
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: can we add a check here that the Content-Type header is the correct one, at least in one of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok


def test_passing_badge(self):
get(Build, project=self.project, version=self.version, success=True)
res = self.client.get(self.badge_url, {'version': self.version.slug})
static_badge = self.get_badge_path('passing')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'passing')
self.assertEqual(res['Content-Type'], 'image/svg+xml')

def test_failing_badge(self):
get(Build, project=self.project, version=self.version, success=False)
res = self.client.get(self.badge_url, {'version': self.version.slug})
static_badge = self.get_badge_path('failing')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'failing')

def test_plastic_failing_badge(self):
get(Build, project=self.project, version=self.version, success=False)
res = self.client.get(self.badge_url, {'version': self.version.slug, 'style': 'plastic'})
static_badge = self.get_badge_path('failing', 'plastic')
self.assertEquals(res.url, static_badge)
self.assertContains(res, 'failing')

# The plastic badge has slightly more rounding
self.assertContains(res, 'rx="4"')

def test_social_passing_badge(self):
get(Build, project=self.project, version=self.version, success=True)
res = self.client.get(self.badge_url, {'version': self.version.slug , 'style': 'social'})
static_badge = self.get_badge_path('passing', 'social')
self.assertEquals(res.url, static_badge)
res = self.client.get(self.badge_url, {'version': self.version.slug, 'style': 'social'})
self.assertContains(res, 'passing')

# The social badge (but not the other badges) has this element
self.assertContains(res, 'rlink')


class TestTags(TestCase):
Expand Down