Skip to content

Commit

Permalink
making addon slugs work in urls (bug 620537)
Browse files Browse the repository at this point in the history
  • Loading branch information
Jeff Balogh committed Dec 21, 2010
1 parent 609de64 commit e0c74ca
Show file tree
Hide file tree
Showing 88 changed files with 648 additions and 530 deletions.
27 changes: 27 additions & 0 deletions apps/addons/decorators.py
@@ -0,0 +1,27 @@
import functools

from django import http
from django.shortcuts import get_object_or_404

from addons.models import Addon


def addon_view(f, qs=Addon.objects):
def wrapper(request, addon_id, *args, **kw):
get = lambda **kw: get_object_or_404(qs, **kw)
if addon_id.isdigit():
addon = get(id=addon_id)
# Don't get in an infinite loop if addon.slug.isdigit().
if addon.slug != addon_id:
url = request.path.replace(addon_id, addon.slug)
if request.GET:
url += '?' + request.GET.urlencode()
return http.HttpResponsePermanentRedirect(url)
else:
addon = get(slug=addon_id)
return f(request, addon, *args, **kw)
return wrapper


def addon_view_factory(qs):
return functools.partial(addon_view, qs=qs)
2 changes: 1 addition & 1 deletion apps/addons/fixtures/addons/addon_228106_info+dev+bio.json
Expand Up @@ -149,7 +149,7 @@
"prerelease": false,
"thankyou_note": 1325266,
"admin_review": false,
"slug": "my-addon-228106",
"slug": "a228106",
"external_software": false,
"highest_status": 1,
"get_satisfaction_company": "",
Expand Down
Expand Up @@ -149,7 +149,7 @@
"prerelease": false,
"thankyou_note": 13252966,
"admin_review": false,
"slug": "my-addon",
"slug": "a228107",
"external_software": false,
"highest_status": 1,
"get_satisfaction_company": "",
Expand Down
2 changes: 1 addition & 1 deletion apps/addons/fixtures/addons/eula+contrib-addon.json
Expand Up @@ -46,7 +46,7 @@
"pk": 11730,
"model": "addons.addon",
"fields": {
"slug": "11730",
"slug": "a11730",
"dev_agreement": 1,
"eula": 372438,
"last_updated": "2009-08-27 14:29:14",
Expand Down
2 changes: 1 addition & 1 deletion apps/addons/fixtures/addons/persona.json
Expand Up @@ -74,7 +74,7 @@
"pk": 15663,
"model": "addons.addon",
"fields": {
"slug": "15663",
"slug": "a15663",
"dev_agreement": 0,
"eula": null,
"last_updated": "2009-03-30 23:17:40",
Expand Down
36 changes: 24 additions & 12 deletions apps/addons/models.py
Expand Up @@ -43,6 +43,11 @@ def get_query_set(self):
qs = qs._clone(klass=query.IndexQuerySet)
return qs.transform(Addon.transformer)

def id_or_slug(self, val):
if isinstance(val, basestring) and not val.isdigit():
return self.filter(slug=val)
return self.filter(id=val)

def public(self):
"""Get public add-ons only"""
return self.filter(self.valid_q([amo.STATUS_PUBLIC]))
Expand Down Expand Up @@ -227,6 +232,12 @@ class Meta:
def __unicode__(self):
return '%s: %s' % (self.id, self.name)

def __init__(self, *args, **kw):
super(Addon, self).__init__(*args, **kw)
# Make sure all add-ons have a slug. save() runs clean_slug.
if self.id and not self.slug:
self.save()

def save(self, **kw):
self.clean_slug()
super(Addon, self).save(**kw)
Expand Down Expand Up @@ -284,12 +295,12 @@ def from_upload(cls, upload, platform):
return addon

def flush_urls(self):
urls = ['*/addon/%d/' % self.id, # Doesn't take care of api
'*/addon/%d/developers/' % self.id,
'*/addon/%d/eula/*' % self.id,
'*/addon/%d/privacy/' % self.id,
'*/addon/%d/versions/*' % self.id,
'*/api/*/addon/%d' % self.id,
urls = ['*/addon/%s/' % self.slug, # Doesn't take care of api
'*/addon/%s/developers/' % self.slug,
'*/addon/%s/eula/*' % self.slug,
'*/addon/%s/privacy/' % self.slug,
'*/addon/%s/versions/*' % self.slug,
'*/api/*/addon/%s' % self.slug,
self.icon_url,
self.thumbnail_url,
]
Expand All @@ -298,21 +309,21 @@ def flush_urls(self):
return urls

def get_url_path(self):
return reverse('addons.detail', args=(self.id,))
return reverse('addons.detail', args=[self.slug])

def meet_the_dev_url(self):
return reverse('addons.meet', args=[self.id])
return reverse('addons.meet', args=[self.slug])

@property
def reviews_url(self):
return reverse('reviews.list', args=(self.id,))
return reverse('reviews.list', args=[self.slug])

def type_url(self):
"""The url for this add-on's AddonType."""
return AddonType(self.type).get_url_path()

def share_url(self):
return reverse('addons.share', args=(self.id,))
return reverse('addons.share', args=[self.slug])

@amo.cached_property(writable=True)
def listed_authors(self):
Expand Down Expand Up @@ -488,7 +499,8 @@ def update_status(self, using=None):
self.update(status=amo.STATUS_NULL)
amo.log(amo.LOG.CHANGE_STATUS, self.get_status_display(), self)

elif not self.versions.using(using).filter(files__isnull=False).exists():
elif not (self.versions.using(using)
.filter(files__isnull=False).exists()):
self.update(status=amo.STATUS_NULL)
amo.log(amo.LOG.CHANGE_STATUS, self.get_status_display(), self)

Expand Down Expand Up @@ -567,7 +579,7 @@ def authors_other_addons(self):
@property
def contribution_url(self, lang=settings.LANGUAGE_CODE,
app=settings.DEFAULT_APP):
return reverse('addons.contribute', args=[self.id])
return reverse('addons.contribute', args=[self.slug])

@property
def thumbnail_url(self):
Expand Down
2 changes: 1 addition & 1 deletion apps/addons/templates/addons/button.html
Expand Up @@ -70,7 +70,7 @@

{% if b.detailed %}
{% if addon.privacy_policy %}
<a class="privacy-policy" href="{{ url('addons.privacy', addon.id) }}">
<a class="privacy-policy" href="{{ url('addons.privacy', addon.slug) }}">
<strong>{{ _('View privacy policy') }}</strong>
</a>
{% endif %}
Expand Down
10 changes: 5 additions & 5 deletions apps/addons/templates/addons/details.html
Expand Up @@ -144,10 +144,10 @@ <h3 id="more-about">{{ _('More about this add-on') }}</h3>
{% if previews|length > 1 %}
<h4>{{ _('Image Gallery') }}</h4>
{% for preview in previews[1:] %}
<a class="screenshot thumbnail" rel="jquery-lightbox"
href="{{ preview.image_url }}" title="{{ preview.caption }}">
<img src="{{ preview.thumbnail_url }}" alt="" />
</a>
<a class="screenshot thumbnail" rel="jquery-lightbox"
href="{{ preview.image_url }}" title="{{ preview.caption }}">
<img src="{{ preview.thumbnail_url }}" alt="" />
</a>
{% endfor %}
{% endif %}

Expand Down Expand Up @@ -308,7 +308,7 @@ <h3 class="compact-bottom">{{ _('Often used with&hellip;')|safe }}</h3>
<ul class="addon-otheraddons">
{% for rec in recommendations %}
<li>
<a href="{{ url('addons.detail', addon_id=rec.id) }}?src=oftenusedwith"
<a href="{{ url('addons.detail', addon_id=rec.slug) }}?src=oftenusedwith"
class="addonitem">{{ rec.name }}</a>
</li>
{% endfor %}
Expand Down
8 changes: 4 additions & 4 deletions apps/addons/templates/addons/eula.html
Expand Up @@ -10,8 +10,8 @@
{% block content %}
<header>
{# L10n: EULA stand for End User License Agreement #}
{{ breadcrumbs([(addon.type_url(), amo.ADDON_TYPES[addon.type] ),
(url('addons.detail',addon.id), addon.name),
{{ breadcrumbs([(addon.type_url(), amo.ADDON_TYPES[addon.type]),
(url('addons.detail', addon.slug), addon.name),
(None, _('EULA'))]) }}
<h2 class="name"{{ addon.name|locale_html }}>
<span>
Expand All @@ -35,12 +35,12 @@ <h3>{{ _('End-User License Agreement') }}</h3>
{{ install_button(addon, version=version, show_contrib=False,
show_eula=False, show_warning=False) }}
<p class="policy-link">
<a href ="{{ url('addons.detail', addon.id) }}">
<a href ="{{ url('addons.detail', addon.slug) }}">
{{ _('Cancel Installation')|f(addon.name) }}
</a>
</p>
<p class="policy-link">
<a href ="{{ url('addons.detail', addon.id) }}">
<a href ="{{ url('addons.detail', addon.slug) }}">
{{ _('Back to {0}...')|f(addon.name) }}
</a>
</p>
Expand Down
4 changes: 2 additions & 2 deletions apps/addons/templates/addons/license.html
Expand Up @@ -8,8 +8,8 @@

{% block content %}
<header>
{{ breadcrumbs([(addon.type_url(), amo.ADDON_TYPES[addon.type] ),
(url('addons.detail',addon.id), addon.name),
{{ breadcrumbs([(addon.type_url(), amo.ADDON_TYPES[addon.type]),
(url('addons.detail', addon.slug), addon.name),
(None, _('License'))]) }}
<h2 class="name"{{ addon.name|locale_html }}>
<span>
Expand Down
2 changes: 1 addition & 1 deletion apps/addons/templates/addons/persona_preview.html
Expand Up @@ -32,7 +32,7 @@ <h6><a href="{{ addon.get_url_path() }}">{{ addon.name }}</a></h6>
addon.created|datetime(dt)) }}
</p>
{% if addon.total_reviews %}
<a href="{{ url('reviews.list', addon.id) }}">
<a href="{{ url('reviews.list', addon.slug) }}">
{{ addon.average_rating|float|stars }}
{{ addon.total_reviews }}</a>
{% else %}
Expand Down
6 changes: 3 additions & 3 deletions apps/addons/templates/addons/privacy.html
Expand Up @@ -13,7 +13,7 @@
<header>
{# L10n: The Privacy Policy for this addon #}
{{ breadcrumbs([(addon.type_url(), amo.ADDON_TYPES[addon.type] ),
(url('addons.detail',addon.id), addon.name),
(url('addons.detail', addon.slug), addon.name),
(None, _('Privacy Policy'))]) }}
<h2 class="name"{{ addon.name|locale_html }}>
<span>
Expand All @@ -27,8 +27,8 @@ <h2 class="name"{{ addon.name|locale_html }}>
<h3>{{ _('Privacy Policy') }}</h3>
<div class="policy-statement">{{ addon.privacy_policy }}</div>
<p class="policy-link">
<a href ="{{ url('addons.detail', addon.id) }}">
<a href ="{{ url('addons.detail', addon.slug) }}">
{{ _('Back to {0}...')|f(addon.name) }}
</a>
</p>
{% endblock content %}
{% endblock content %}
2 changes: 1 addition & 1 deletion apps/addons/templates/addons/report_abuse.html
@@ -1,5 +1,5 @@
<div id="addon" class="primary">
<form method="post" action="{{ url('addons.abuse', addon.pk) }}">
<form method="post" action="{{ url('addons.abuse', addon.slug) }}">
<fieldset class="abuse">
{% if hide %}
<legend><a href="{{ remora_url('developers/docs/policies/contact') }}"
Expand Down
4 changes: 2 additions & 2 deletions apps/addons/templates/addons/review_add_box.html
Expand Up @@ -11,7 +11,7 @@ <h3>{{ _('What do you think?') }}</h3>
</p>
{% endif %}

<form disabled method="post" action="{{ url('reviews.add', addon.id) }}">
<form disabled method="post" action="{{ url('reviews.add', addon.slug) }}">
{% set attrs = {} if user.is_authenticated() else {'disabled': 'disabled'} %}
{{ csrf() }}
{{ field(review_form.body, _('Review:'), **attrs) }}
Expand All @@ -38,7 +38,7 @@ <h3>{{ _('What do you think?') }}</h3>
{# TODO reverse url #}
<p><a href="{{ remora_url('/pages/review_guide') }}">{{ _('Review Guidelines') }}</a></p>
<p>
<a href="{{ url('reviews.add', addon.id) }}">
<a href="{{ url('reviews.add', addon.slug) }}">
{{ _('Detailed Review') }}</a>
</p>
</div>
Expand Down
2 changes: 1 addition & 1 deletion apps/addons/templates/addons/review_list_box.html
Expand Up @@ -23,7 +23,7 @@ <h5>{{ review.title }}</h5>
{% endfor %}
{% if addon %}
<p>
<a class="more-info" href="{{ url('reviews.list', addon.id) }}">
<a class="more-info" href="{{ url('reviews.list', addon.slug) }}">
{% trans num=addon.total_reviews, cnt=addon.total_reviews|numberfmt %}
See all reviews of this add-on
{% pluralize %}
Expand Down
2 changes: 1 addition & 1 deletion apps/addons/templates/addons/support_addon.html
@@ -1,5 +1,5 @@
{% if addon.takes_contributions %}
{% set base = url('addons.contribute', addon.id)|urlparams(src='direct') %}
{% set base = url('addons.contribute', addon.slug)|urlparams(src='direct') %}
<div class="contribute">
{% if addon.suggested_amount is not none %}
{% trans url=base|urlparams(type='suggested')|escape,
Expand Down
83 changes: 83 additions & 0 deletions apps/addons/tests/test_decorators.py
@@ -0,0 +1,83 @@
from django import http

import mock
import test_utils
from nose.tools import eq_

from addons.models import Addon
from addons import decorators as dec


class TestAddonView(test_utils.TestCase):

def setUp(self):
self.addon = Addon.objects.create(slug='x', type=1)
self.func = mock.Mock()
self.func.return_value = mock.sentinel.OK
self.view = dec.addon_view(self.func)
self.request = mock.Mock()
self.slug_path = '/addon/%s/reviews' % self.addon.slug
self.request.path = self.id_path = '/addon/%s/reviews' % self.addon.id
self.request.GET = {}

def test_301_by_id(self):
r = self.view(self.request, str(self.addon.id))
eq_(r.status_code, 301)
eq_(r['Location'], self.slug_path)

def test_301_with_querystring(self):
self.request.GET = mock.Mock()
self.request.GET.urlencode.return_value = 'q=1'
r = self.view(self.request, str(self.addon.id))
eq_(r.status_code, 301)
eq_(r['Location'], self.slug_path + '?q=1')

def test_200_by_slug(self):
r = self.view(self.request, self.addon.slug)
eq_(r, mock.sentinel.OK)

def test_404_by_id(self):
with self.assertRaises(http.Http404):
self.view(self.request, str(self.addon.id * 2))

def test_404_by_slug(self):
with self.assertRaises(http.Http404):
self.view(self.request, self.addon.slug + 'xx')

def test_alternate_qs_301_by_id(self):
qs = Addon.objects.filter(type=1)
view = dec.addon_view_factory(qs=qs)(self.func)
r = view(self.request, str(self.addon.id))
eq_(r.status_code, 301)
eq_(r['Location'], self.slug_path)

def test_alternate_qs_200_by_slug(self):
qs = Addon.objects.filter(type=1)
view = dec.addon_view_factory(qs=qs)(self.func)
r = view(self.request, self.addon.slug)
eq_(r, mock.sentinel.OK)

def test_alternate_qs_404_by_id(self):
qs = Addon.objects.filter(type=2)
view = dec.addon_view_factory(qs=qs)(self.func)
with self.assertRaises(http.Http404):
view(self.request, str(self.addon.id))

def test_alternate_qs_404_by_slug(self):
qs = Addon.objects.filter(type=2)
view = dec.addon_view_factory(qs=qs)(self.func)
with self.assertRaises(http.Http404):
view(self.request, self.addon.slug)

def test_addon_no_slug(self):
a = Addon.objects.create(type=1, name='xxxx')
r = self.view(self.request, a.slug)
eq_(r, mock.sentinel.OK)

def test_slug_isdigit(self):
a = Addon.objects.create(type=1, name='xxxx')
a.update(slug=str(a.id))
r = self.view(self.request, a.slug)
eq_(r, mock.sentinel.OK)
request, addon = self.func.call_args[0]
eq_(addon, a)
2 changes: 1 addition & 1 deletion apps/addons/tests/test_helpers.py
Expand Up @@ -43,7 +43,7 @@ def test_flags(self):
eq_(flag(ctx, featured), '<h5 class="flag">Featured</h5>')

def test_support_addon(self):
a = Addon(id=12)
a = Addon(id=12, type=1)
eq_(support_addon(a), '')

a.wants_contributions = a.paypal_id = True
Expand Down

0 comments on commit e0c74ca

Please sign in to comment.