-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Serve badges directly from local filesystem #4561
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
Changes from 1 commit
2e99a1f
53376a2
1f39313
0e19c18
2b3c3b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,25 +115,38 @@ 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: | ||
version_builds = version.builds.filter(type='html', | ||
state='finished').order_by('-date') | ||
if version_builds.exists(): | ||
last_build = version_builds[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose if we are cleaning up the existing code a bit, I might as well clean this up too. |
||
if last_build.success: | ||
file_path = static(badge_path % 'passing') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely want to get rid of the static call. |
||
else: | ||
file_path = static(badge_path % 'failing') | ||
|
||
with open(file_path) as fd: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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') | ||
|
@@ -436,32 +430,38 @@ 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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: can we add a check here that the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
||
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): | ||
|
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 file was previously autolinted and this line reverts some of the autolinting -- you should be able to resolve with a
pre-commit
pass.