Submitter:	Christian Hammond	Reviewers
Branch:	trunk	Groups:	reviewboard
Bugs:	458, 459	People:
Change Number:	None	Repository:	Review Board SVN

Description:

We've been using the old get_absolute_url() function in our templates, which works for the most part but has limitations. Using it on a User object doesn't work so well when installed in a subdirectory, requiring that we prepend SITE_ROOT and then slice the first "/". This gets ugly fast.

We've also been hard-coding other paths ("/account/login/", "/dashboard/", etc.), requiring prepending SITE_ROOT again.

Django's {% url %} tag provides a cleaner solution, allowing us to give certain pages names that we can refer to. Now something like {{SITE_ROOT}}{{submitter.get_absolute_url|slice:"1"}} turns into {% url user submitter %}.

This in theory could also give extensions in the future the ability to override certain pages if they wanted.

The result of this change is that we use get_absolute_url() a lot less for some things and never for users. We could move the rest of the get_absolute_url() calls in the future if we choose, though what we have works for now. We've also eliminated most references to SITE_ROOT in the templates.

There's some misc. fixes and cleanups in here too, such as using djblets's new user_displayname filter.

Testing Done:

Made sure every page I tested worked properly in subdirectory installs. All the user links I could find now point to the correct location.

Diff revision 1 (Latest)

Files Changed:

/trunk/reviewboard/urls.py: 14 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 ]
/trunk/reviewboard/accounts/decorators.py: 2 changes [ 1 2 ]
/trunk/reviewboard/accounts/views.py: 4 changes [ 1 2 3 4 ]
/trunk/reviewboard/iphone/urls.py: 17 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 ]
/trunk/reviewboard/reviews/datagrids.py: 2 changes [ 1 2 ]
/trunk/reviewboard/reviews/urls.py: 4 changes [ 1 2 3 4 ]
/trunk/reviewboard/reviews/views.py: 3 changes [ 1 2 3 ]
/trunk/reviewboard/reviews/templatetags/reviewtags.py: 2 changes [ 1 2 ]
/trunk/reviewboard/templates/base.html: 9 changes [ 1 2 3 4 5 6 7 8 9 ]
/trunk/reviewboard/templates/base_feeds.html: 1 change [ 1 ]
/trunk/reviewboard/templates/accounts/login.html: 1 change [ 1 ]
/trunk/reviewboard/templates/diffviewer/view_diff.html: 2 changes [ 1 2 ]
/trunk/reviewboard/templates/iphone/base.html: 3 changes [ 1 2 3 ]
/trunk/reviewboard/templates/iphone/dashboard.html: 4 changes [ 1 2 3 4 ]
/trunk/reviewboard/templates/iphone/diff_files.html: 1 change [ 1 ]
/trunk/reviewboard/templates/iphone/group_list.html: 1 change [ 1 ]
/trunk/reviewboard/templates/iphone/login.html: 1 change [ 1 ]
/trunk/reviewboard/templates/iphone/review_request_detail.html: 5 changes [ 1 2 3 4 5 ]
/trunk/reviewboard/templates/iphone/review_request_list.html: 1 change [ 1 ]
/trunk/reviewboard/templates/iphone/submitter_list.html: 1 change [ 1 ]
/trunk/reviewboard/templates/reviews/dashboard_entry.html: 1 change [ 1 ]
/trunk/reviewboard/templates/reviews/review_detail.html: 3 changes [ 1 2 3 ]
/trunk/reviewboard/templates/reviews/review_reply.html: 2 changes [ 1 2 ]
/trunk/reviewboard/templates/reviews/review_request_box.html: 3 changes [ 1 2 3 ]
/trunk/reviewboard/templates/reviews/search.html: 3 changes [ 1 2 3 ]

Expand changes

 import os.path
 from django.conf import settings
-from django.conf.urls.defaults import patterns, include, handler404, handler500
+from django.conf.urls.defaults import patterns, include, url
 from reviewboard.admin.checks import check_updates_required
 from reviewboard.reviews.feeds import RssReviewsFeed, AtomReviewsFeed, \
                                       RssSubmitterReviewsFeed, \
                                       AtomSubmitterReviewsFeed, \
     # reviewboard.reviews.views
     urlpatterns += patterns('reviewboard.reviews.views',
         # Review request browsing
-        (r'^dashboard/$', 'dashboard'),
+        url(r'^dashboard/$', 'dashboard', name="dashboard"),
         # Users
-        (r'^users/$', 'submitter_list'),
+        url(r'^users/$', 'submitter_list', name="all-users"),
-        (r'^users/(?P<username>[A-Za-z0-9_\-\.]+)/$', 'submitter'),
+        url(r'^users/(?P<username>[A-Za-z0-9_\-\.]+)/$', 'submitter',
             name="user"),
         # Groups
-        (r'^groups/$', 'group_list'),
+        url(r'^groups/$', 'group_list', name="all-groups"),
-        (r'^groups/(?P<name>[A-Za-z0-9_-]+)/$', 'group'),
+        url(r'^groups/(?P<name>[A-Za-z0-9_-]+)/$', 'group', name="group"),
+    )
     # django.contrib
     urlpatterns += patterns('django.contrib',
        # Feeds
-        (r'^feeds/rss/(?P<url>.*)/$', 'syndication.views.feed',
+        url(r'^feeds/rss/(?P<url>.*)/$', 'syndication.views.feed',
-         {'feed_dict': rss_feeds}),
+            {'feed_dict': rss_feeds},
-        (r'^feeds/atom/(?P<url>.*)/$', 'syndication.views.feed',
+            name="rss-feed"),
-         {'feed_dict': atom_feeds}),
+        url(r'^feeds/atom/(?P<url>.*)/$', 'syndication.views.feed',
-        (r'^account/logout/$', 'auth.views.logout',
+           {'feed_dict': atom_feeds},
-         {'next_page': settings.LOGIN_URL}),
+           name="atom-feed"),
         url(r'^account/logout/$', 'auth.views.logout',
             {'next_page': settings.LOGIN_URL},
             name="logout")
+    )
     # And the rest ...
     urlpatterns += patterns('',
-        (r'^$', 'django.views.generic.simple.redirect_to',
+        url(r'^$', 'django.views.generic.simple.redirect_to',
-         {'url': 'dashboard/'}),
+            {'url': 'dashboard/'},
             name="root"),
         # Authentication and accounts
-        (r'^account/login/$', 'djblets.auth.views.login',
+        url(r'^account/login/$', 'djblets.auth.views.login',
-         {'next_page': settings.SITE_ROOT + 'dashboard/',
+            {'next_page': settings.SITE_ROOT + 'dashboard/',
-          'extra_context': {'BUILTIN_AUTH': settings.BUILTIN_AUTH}}),
+             'extra_context': {'BUILTIN_AUTH': settings.BUILTIN_AUTH}},
-        (r'^account/preferences/$', 'reviewboard.accounts.views.user_preferences',),
+            name="login"),
         url(r'^account/preferences/$',
             'reviewboard.accounts.views.user_preferences',
             name="user-preferences"),
         # This must be last.
         (r'^iphone/', include('reviewboard.iphone.urls')),
+    )
     if settings.BUILTIN_AUTH:
         urlpatterns += patterns('',
-            (r'^account/register/$', 'djblets.auth.views.register',
+            url(r'^account/register/$', 'djblets.auth.views.register',
-             {'next_page': settings.SITE_ROOT + 'dashboard/'}),
+                {'next_page': settings.SITE_ROOT + 'dashboard/'},
                 name="register"),
+        )
     else:
         urlpatterns += patterns('',
             (r'^account/register/$',
              'django.views.generic.simple.redirect_to',
              {'url': settings.SITE_ROOT + 'account/login/'}))

 from urllib import quote
 from django.conf import settings
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseRedirect
 from djblets.auth.util import login_required
 from djblets.util.decorators import simple_decorator
             if profile.first_time_setup_done:
                 return view_func(request, *args, **kwargs)
         except Profile.DoesNotExist:
             pass
-        return HttpResponseRedirect("%saccount/preferences/?%s=%s" %
+        return HttpResponseRedirect("%s?%s=%s" %
-                                    (settings.SITE_ROOT,
+                                    (reverse("user-preferences"),
                                      REDIRECT_FIELD_NAME,
                                      quote(request.get_full_path())))
     return _check_valid_prefs

 import time
 from sha import sha
 from django.conf import settings
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.contrib.auth.models import get_hexdigest
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseRedirect
 from django.shortcuts import render_to_response
 from django.template.context import RequestContext
 from djblets.auth.util import login_required
 from reviewboard.accounts.forms import PreferencesForm
 from reviewboard.accounts.models import Profile
 @login_required
 def user_preferences(request, template_name='accounts/prefs.html'):
-    redirect_to = request.REQUEST.get(REDIRECT_FIELD_NAME, settings.SITE_ROOT)
+    redirect_to = request.REQUEST.get(REDIRECT_FIELD_NAME,
                                       reverse("dashboard"))
     profile, profile_is_new = \
         Profile.objects.get_or_create(user=request.user)
     must_configure = not profile.first_time_setup_done
     profile.save()
             profile.syntax_highlighting = \
                 form.cleaned_data['syntax_highlighting']
             profile.save()
             return HttpResponseRedirect(redirect_to)
     else:
         form = PreferencesForm({
             'settings': settings,
             'redirect_to': redirect_to,
             'first_name': request.user.first_name,

 from django.conf import settings
-from django.conf.urls.defaults import patterns
+from django.conf.urls.defaults import patterns, url
 urlpatterns = patterns('',
     (r'^$', 'django.views.generic.simple.direct_to_template',
      {'template': 'iphone/base.html'}),
     # Accounts
-    (r'^account/login/$', 'djblets.auth.views.login',
+    url(r'^account/login/$', 'djblets.auth.views.login',
-     {'next_page': '/iphone/',
+        {'next_page': settings.SITE_ROOT + '/iphone/',
-      'template_name': 'iphone/login.html',
+         'template_name': 'iphone/login.html',
-      'extra_context': {'BUILTIN_AUTH': settings.BUILTIN_AUTH}}),
+         'extra_context': {'BUILTIN_AUTH': settings.BUILTIN_AUTH}},
-    (r'^account/logout/$', 'django.contrib.auth.views.logout',
+        name="iphone-login"),
-     {'next_page': '/iphone/'}),
+    url(r'^account/logout/$', 'django.contrib.auth.views.logout',
-    (r'^account/preferences/$', 'reviewboard.accounts.views.user_preferences',
+        {'next_page': settings.SITE_ROOT + 'iphone/'},
-     {'template_name': 'iphone/user_prefs.html'}),
+        name="iphone-logout"),
     url(r'^account/preferences/$',
         'reviewboard.accounts.views.user_preferences',
         {'template_name': 'iphone/user_prefs.html'},
         name="iphone-user-preferences"),
+)
 urlpatterns += patterns('reviewboard.reviews.views',
     # Dashboard
-    (r'^dashboard/$', 'dashboard',
+    url(r'^dashboard/$', 'dashboard',
-     {'template_name': 'iphone/dashboard.html'}),
+        {'template_name': 'iphone/dashboard.html'},
-    (r'^dashboard/list/$', 'dashboard',
+        name="iphone-dashboard"),
-     {'template_name': 'iphone/dashboard_list.html'}),
+    url(r'^dashboard/list/$', 'dashboard',
         {'template_name': 'iphone/dashboard_list.html'},
         name="iphone-dashboard-list"),
     # Users
-    (r'^users/$', 'submitter_list',
+    url(r'^users/$', 'submitter_list',
-     {'template_name': 'iphone/submitter_list.html'}),
+        {'template_name': 'iphone/submitter_list.html'},
-    (r'^users/(?P<username>[A-Za-z0-9_-]+)/$', 'submitter',
+        name="iphone-users"),
-     {'template_name': 'iphone/review_request_list.html'}),
+    url(r'^users/(?P<username>[A-Za-z0-9_-]+)/$', 'submitter',
         {'template_name': 'iphone/review_request_list.html'},
         name="iphone-user"),
     # Groups
-    (r'^groups/$', 'group_list',
+    url(r'^groups/$', 'group_list',
-     {'template_name': 'iphone/group_list.html'}),
+        {'template_name': 'iphone/group_list.html'},
-    (r'^groups/(?P<name>[A-Za-z0-9_-]+)/$', 'group',
+        name="iphone-groups"),
-     {'template_name': 'iphone/review_request_list.html'}),
+    url(r'^groups/(?P<name>[A-Za-z0-9_-]+)/$', 'group',
         {'template_name': 'iphone/review_request_list.html'},
         name="iphone-group"),
     # Review Requests
-    (r'^r/$', 'all_review_requests',
+    url(r'^r/$', 'all_review_requests',
-     {'template_name': 'iphone/review_request_list.html'}),
+        {'template_name': 'iphone/review_request_list.html'},
         name="iphone-all-review-requests"),
     (r'^r/(?P<review_request_id>[0-9]+)/$', 'review_detail',
-     {'template_name': 'iphone/review_request_detail.html'}),
+    url(r'^r/(?P<review_request_id>[0-9]+)/$', 'review_detail',
         {'template_name': 'iphone/review_request_detail.html'},
         name="iphone-review-request"),
     # Diffs
-    (r'^r/(?P<review_request_id>[0-9]+)/diff/$', 'diff',
+    url(r'^r/(?P<review_request_id>[0-9]+)/diff/$', 'diff',
-     {'template_name': 'iphone/diff_files.html'}),
+        {'template_name': 'iphone/diff_files.html'},
         name="iphone-diff"),
-    (r'^r/(?P<review_request_id>[0-9]+)/diff/(?P<revision>[0-9]+)/(?P<filediff_id>[0-9]+)/$',
+    url(r'^r/(?P<review_request_id>[0-9]+)/diff/(?P<revision>[0-9]+)/(?P<filediff_id>[0-9]+)/$',
-     'diff_fragment',
+        'diff_fragment',
-     {'collapseall': True,
+        {'collapseall': True,
-      'template_name': 'iphone/diff.html'}),
+         'template_name': 'iphone/diff.html'},
         name="iphone-diff-fragment"),
+)

 from django.conf import settings
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.db.models import Q
 from django.template import Template
 from django.template.context import RequestContext
 from django.template.defaultfilters import date, timesince
 from django.template.loader import render_to_string
             "username", "fullname", "pending_reviews"
+        ]
     @staticmethod
     def link_to_object(obj, value):
-        return settings.SITE_ROOT + obj.get_absolute_url()[1:]
+        return reverse("user", obj)
 class GroupDataGrid(DataGrid):
     """
     A datagrid showing a list of review groups.

-from django.conf.urls.defaults import patterns
+from django.conf.urls.defaults import patterns, url
 urlpatterns = patterns('reviewboard.reviews.views',
-    (r'^$', 'all_review_requests'),
+    url(r'^$', 'all_review_requests', name="all-review-requests"),
     # Review request creation
-    (r'^new/$', 'new_review_request'),
+    url(r'^new/$', 'new_review_request', name="new-review-request"),
     # Review request detail
     (r'^(?P<review_request_id>[0-9]+)/$', 'review_detail',
      {'template_name': 'reviews/review_detail.html'}),
      'preview_review_email'),
     (r'^(?P<review_request_id>[0-9]+)/reviews/(?P<review_id>[0-9]+)/replies/(?P<reply_id>[0-9]+)/preview-email/$',
      'preview_reply_email'),
     # Search
-    (r'^search/$', 'search'),
+    url(r'^search/$', 'search', name="search"),
+)

 from datetime import datetime
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.contrib.sites.models import Site
 from django.core.urlresolvers import reverse
 from django.db.models import Q
 from django.http import HttpResponse, HttpResponseRedirect, Http404, \
                         HttpResponseForbidden
 from django.shortcuts import get_object_or_404, render_to_response
 from django.template.context import RequestContext
         # This should never happen under normal circumstances
         raise Exception('Error when setting review status: unknown status code')
     review_request.save()
     if action == 'discard':
-        return HttpResponseRedirect(settings.SITE_ROOT + 'dashboard/')
+        return HttpResponseRedirect(reverse("dashboard"))
     else:
         return HttpResponseRedirect(review_request.get_absolute_url())
 @check_login_required
         # FIXME: show something useful
         raise Http404
     if not query:
         # FIXME: I'm not super thrilled with this
-        return HttpResponseRedirect('/')
+        return HttpResponseRedirect(reverse("root"))
     import lucene
     # We may have already initialized lucene
     try:

         else:
             count = len(review_requests)
     return {
         'MEDIA_URL': settings.MEDIA_URL,
-        'SITE_ROOT': settings.SITE_ROOT,
         'level': level,
         'text': text,
         'view': view,
         'group': group,
         'count': count,
     return render_to_string('reviews/star.html', {
         'object': obj_info,
         'starred': int(starred),
         'user': user,
         'MEDIA_URL': settings.MEDIA_URL,
-        'SITE_ROOT': settings.SITE_ROOT,
     })

/trunk/reviewboard/templates/base.html
Revision 1295		New Change
...	30 lines hidden [Expand]
31	{% block bodytag %}	31	{% block bodytag %}
32	<body>	32	<body>

Review Board

beta

This change has been marked as submitted.

Redo our URL scheme to use {% url %} tags

Diff revision 1 (Latest)