Commit 554f0601 authored by Ramiro Morales's avatar Ramiro Morales
Browse files

Stopped unconditionally reversing admin model add/change URLs. 

Starting with [16857] this could cause HTTP 500 errors when
`ModelAdmin.get_urls()` has been customized to the point it doesn't
provide these standard URLs.

Fixes #17333. Refs #15294.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@17237 bcc190cf-cafb-0310-a4f2-bffc1f526a37
parent 259ebcde

django/contrib/admin/sites.py

+21 −5
Original line number Diff line number Diff line
@@ -7,7 +7,7 @@ from django.contrib.contenttypes import views as contenttype_views 
from django.views.decorators.csrf import csrf_protect

from django.db.models.base import ModelBase

from django.core.exceptions import ImproperlyConfigured

from django.core.urlresolvers import reverse

from django.core.urlresolvers import reverse, NoReverseMatch

from django.template.response import TemplateResponse

from django.utils.safestring import mark_safe

from django.utils.text import capfirst
@@ -342,10 +342,18 @@ class AdminSite(object): 
                    info = (app_label, model._meta.module_name)

                    model_dict = {

                        'name': capfirst(model._meta.verbose_name_plural),

                        'admin_url': reverse('admin:%s_%s_changelist' % info, current_app=self.name),

                        'add_url': reverse('admin:%s_%s_add' % info, current_app=self.name),

                        'perms': perms,

                    }

                    if perms.get('change', False):

                        try:

                            model_dict['admin_url'] = reverse('admin:%s_%s_changelist' % info, current_app=self.name)

                        except NoReverseMatch:

                            pass

                    if perms.get('add', False):

                        try:

                            model_dict['add_url'] = reverse('admin:%s_%s_add' % info, current_app=self.name)

                        except NoReverseMatch:

                            pass

                    if app_label in app_dict:

                        app_dict[app_label]['models'].append(model_dict)

                    else:
@@ -388,10 +396,18 @@ class AdminSite(object): 
                        info = (app_label, model._meta.module_name)

                        model_dict = {

                            'name': capfirst(model._meta.verbose_name_plural),

                            'admin_url': reverse('admin:%s_%s_changelist' % info, current_app=self.name),

                            'add_url': reverse('admin:%s_%s_add' % info, current_app=self.name),

                            'perms': perms,

                        }

                        if perms.get('change', False):

                            try:

                                model_dict['admin_url'] = reverse('admin:%s_%s_changelist' % info, current_app=self.name)

                            except NoReverseMatch:

                                pass

                        if perms.get('add', False):

                            try:

                                model_dict['add_url'] = reverse('admin:%s_%s_add' % info, current_app=self.name)

                            except NoReverseMatch:

                                pass

                        if app_dict:

                            app_dict['models'].append(model_dict),

                        else:

django/contrib/admin/templates/admin/index.html

+3 −3
Original line number Diff line number Diff line
@@ -19,19 +19,19 @@ 
        <caption><a href="{{ app.app_url }}" class="section">{% blocktrans with name=app.name %}{{ name }}{% endblocktrans %}</a></caption>

        {% for model in app.models %}

            <tr>

            {% if model.perms.change %}

            {% if model.admin_url %}

                <th scope="row"><a href="{{ model.admin_url }}">{{ model.name }}</a></th>

            {% else %}

                <th scope="row">{{ model.name }}</th>

            {% endif %}



            {% if model.perms.add %}

            {% if model.add_url %}

                <td><a href="{{ model.add_url }}" class="addlink">{% trans 'Add' %}</a></td>

            {% else %}

                <td>&nbsp;</td>

            {% endif %}



            {% if model.perms.change %}

            {% if model.admin_url %}

                <td><a href="{{ model.admin_url }}" class="changelink">{% trans 'Change' %}</a></td>

            {% else %}

                <td>&nbsp;</td>

tests/regressiontests/admin_views/admin.py

+15 −2
Original line number Diff line number Diff line 
# -*- coding: utf-8 -*-

from __future__ import absolute_import



import datetime

import tempfile

import os
@@ -10,8 +9,10 @@ from django.contrib import admin 
from django.contrib.admin.views.main import ChangeList

from django.core.files.storage import FileSystemStorage

from django.core.mail import EmailMessage

from django.conf.urls import patterns, url

from django.db import models

from django.forms.models import BaseModelFormSet

from django.http import HttpResponse



from .models import (Article, Chapter, Account, Media, Child, Parent, Picture,

    Widget, DooHickey, Grommet, Whatsit, FancyDoodad, Category, Link,
@@ -24,7 +25,7 @@ from .models import (Article, Chapter, Account, Media, Child, Parent, Picture, 
    CoverLetter, Story, OtherStory, Book, Promo, ChapterXtra1, Pizza, Topping,

    Album, Question, Answer, ComplexSortedPerson, PrePopulatedPostLargeSlug,

    AdminOrderedField, AdminOrderedModelMethod, AdminOrderedAdminMethod,

    AdminOrderedCallable)

    AdminOrderedCallable, Report)





def callable_year(dt_value):
@@ -499,6 +500,17 @@ class AdminOrderedCallableAdmin(admin.ModelAdmin): 
    ordering = ('order',)

    list_display = ('stuff', admin_ordered_callable)



class ReportAdmin(admin.ModelAdmin):

    def extra(self, request):

        return HttpResponse()



    def get_urls(self):

        # Corner case: Don't call parent implementation

        return patterns('',

            url(r'^extra/$',

                self.extra,

                name='cable_extra'),

        )



site = admin.AdminSite(name="admin")

site.register(Article, ArticleAdmin)
@@ -543,6 +555,7 @@ site.register(Paper, PaperAdmin) 
site.register(CoverLetter, CoverLetterAdmin)

site.register(Story, StoryAdmin)

site.register(OtherStory, OtherStoryAdmin)

site.register(Report, ReportAdmin)



# We intentionally register Promo and ChapterXtra1 but not Chapter nor ChapterXtra2.

# That way we cover all four cases:

tests/regressiontests/admin_views/models.py

+6 −0
Original line number Diff line number Diff line
@@ -566,3 +566,9 @@ class AdminOrderedAdminMethod(models.Model): 
class AdminOrderedCallable(models.Model):

    order = models.IntegerField()

    stuff = models.CharField(max_length=200)



class Report(models.Model):

    title = models.CharField(max_length=100)



    def __unicode__(self):

        return self.title

tests/regressiontests/admin_views/tests.py

+33 −1
Original line number Diff line number Diff line
@@ -38,7 +38,8 @@ from .models import (Article, BarAccount, CustomArticle, EmptyModel, FooAccount, 
    Book, Promo, WorkHour, Employee, Question, Answer, Inquisition, Actor,

    FoodDelivery, RowLevelChangePermissionModel, Paper, CoverLetter, Story,

    OtherStory, ComplexSortedPerson, Parent, Child, AdminOrderedField,

    AdminOrderedModelMethod, AdminOrderedAdminMethod, AdminOrderedCallable)

    AdminOrderedModelMethod, AdminOrderedAdminMethod, AdminOrderedCallable,

    Report)





ERROR_MESSAGE = "Please enter the correct username and password \
@@ -1090,6 +1091,37 @@ class AdminViewPermissionsTest(TestCase): 
        self.assertContains(response, 'id="login-form"')





class AdminViewsNoUrlTest(TestCase):

    """Regression test for #17333"""



    urls = "regressiontests.admin_views.urls"

    fixtures = ['admin-views-users.xml']



    def setUp(self):

        opts = Report._meta

        # User who can change Reports

        change_user = User.objects.get(username='changeuser')

        change_user.user_permissions.add(get_perm(Report,

            opts.get_change_permission()))



        # login POST dict

        self.changeuser_login = {

            REDIRECT_FIELD_NAME: '/test_admin/admin/',

            LOGIN_FORM_KEY: 1,

            'username': 'changeuser',

            'password': 'secret',

        }



    def test_no_standard_modeladmin_urls(self):

        """Admin index views don't break when user's ModelAdmin removes standard urls"""

        self.client.get('/test_admin/admin/')

        self.client.post('/test_admin/admin/', self.changeuser_login)

        r = self.client.get('/test_admin/admin/')

        # we shouldn' get an 500 error caused by a NoReverseMatch

        self.assertEqual(r.status_code, 200)

        self.client.get('/test_admin/admin/logout/')





class AdminViewDeletedObjectsTest(TestCase):

    urls = "regressiontests.admin_views.urls"

    fixtures = ['admin-views-users.xml', 'deleted-objects.xml']