From 3c0300405009b82b52fd15483371097221662fcd Mon Sep 17 00:00:00 2001 From: Rainer Koirikivi Date: Sun, 18 Aug 2013 00:05:13 +0200 Subject: [PATCH] Fixed #20640 -- Avoided NoReverseMatch in get_deleted_objects The default delete action resulted in a NoReverseMatch if it were to list any Model with a ModelAdmin with `get_urls` overridden to remove the change url. Catching the error and not displaying the link in that case, as was already done for models with no registered admins. Thanks Keryn Knight for the report. --- django/contrib/admin/util.py | 23 +++++++++++++++-------- tests/admin_views/admin.py | 10 +++++++++- tests/admin_views/models.py | 6 ++++++ tests/admin_views/tests.py | 21 +++++++++++++++++++-- 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/django/contrib/admin/util.py b/django/contrib/admin/util.py index b31756d762..a36c8c13c8 100644 --- a/django/contrib/admin/util.py +++ b/django/contrib/admin/util.py @@ -16,7 +16,7 @@ from django.utils import timezone from django.utils.encoding import force_str, force_text, smart_text from django.utils import six from django.utils.translation import ungettext -from django.core.urlresolvers import reverse +from django.core.urlresolvers import reverse, NoReverseMatch def lookup_needs_distinct(opts, lookup_path): """ @@ -113,12 +113,20 @@ def get_deleted_objects(objs, opts, user, admin_site, using): has_admin = obj.__class__ in admin_site._registry opts = obj._meta + no_edit_link = '%s: %s' % (capfirst(opts.verbose_name), + force_text(obj)) + if has_admin: - admin_url = reverse('%s:%s_%s_change' - % (admin_site.name, - opts.app_label, - opts.model_name), - None, (quote(obj._get_pk_val()),)) + try: + admin_url = reverse('%s:%s_%s_change' + % (admin_site.name, + opts.app_label, + opts.model_name), + None, (quote(obj._get_pk_val()),)) + except NoReverseMatch: + # Change url doesn't exist -- don't display link to edit + return no_edit_link + p = '%s.%s' % (opts.app_label, get_permission_codename('delete', opts)) if not user.has_perm(p): @@ -131,8 +139,7 @@ def get_deleted_objects(objs, opts, user, admin_site, using): else: # Don't display link to edit, because it either has no # admin or is edited inline. - return '%s: %s' % (capfirst(opts.verbose_name), - force_text(obj)) + return no_edit_link to_delete = collector.nested(format_callback) diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index df8ced949e..193b64505c 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -29,7 +29,7 @@ from .models import (Article, Chapter, Account, Media, Child, Parent, Picture, Album, Question, Answer, ComplexSortedPerson, PluggableSearchPerson, PrePopulatedPostLargeSlug, AdminOrderedField, AdminOrderedModelMethod, AdminOrderedAdminMethod, AdminOrderedCallable, Report, Color2, UnorderedObject, MainPrepopulated, - RelatedPrepopulated, UndeletableObject, UserMessenger, Simple, Choice, + RelatedPrepopulated, UndeletableObject, UnchangeableObject, UserMessenger, Simple, Choice, ShortMessage, Telegram) @@ -656,6 +656,13 @@ class UndeletableObjectAdmin(admin.ModelAdmin): return super(UndeletableObjectAdmin, self).change_view(*args, **kwargs) +class UnchangeableObjectAdmin(admin.ModelAdmin): + def get_urls(self): + # Disable change_view, but leave other urls untouched + urlpatterns = super(UnchangeableObjectAdmin, self).get_urls() + return [p for p in urlpatterns if not p.name.endswith("_change")] + + def callable_on_unknown(obj): return obj.unknown @@ -741,6 +748,7 @@ site.register(Report, ReportAdmin) site.register(MainPrepopulated, MainPrepopulatedAdmin) site.register(UnorderedObject, UnorderedObjectAdmin) site.register(UndeletableObject, UndeletableObjectAdmin) +site.register(UnchangeableObject, UnchangeableObjectAdmin) # We intentionally register Promo and ChapterXtra1 but not Chapter nor ChapterXtra2. # That way we cover all four cases: diff --git a/tests/admin_views/models.py b/tests/admin_views/models.py index c63ac3b927..1accf46819 100644 --- a/tests/admin_views/models.py +++ b/tests/admin_views/models.py @@ -674,6 +674,12 @@ class UndeletableObject(models.Model): """ name = models.CharField(max_length=255) +class UnchangeableObject(models.Model): + """ + Model whose change_view is disabled in admin + Refs #20640. + """ + class UserMessenger(models.Model): """ Dummy class for testing message_user functions on ModelAdmin diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 8be623f4ca..8f83324a37 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -15,7 +15,6 @@ from django.core import mail from django.core.files import temp as tempfile from django.core.urlresolvers import reverse # Register auth models with the admin. -from django.contrib import admin from django.contrib.auth import get_permission_codename from django.contrib.admin.helpers import ACTION_CHECKBOX_NAME from django.contrib.admin.models import LogEntry, DELETION @@ -51,7 +50,7 @@ from .models import (Article, BarAccount, CustomArticle, EmptyModel, FooAccount, OtherStory, ComplexSortedPerson, PluggableSearchPerson, Parent, Child, AdminOrderedField, AdminOrderedModelMethod, AdminOrderedAdminMethod, AdminOrderedCallable, Report, MainPrepopulated, RelatedPrepopulated, UnorderedObject, - Simple, UndeletableObject, Choice, ShortMessage, Telegram) + Simple, UndeletableObject, UnchangeableObject, Choice, ShortMessage, Telegram) from .admin import site, site2 @@ -2422,6 +2421,24 @@ class AdminActionsTest(TestCase): self.assertContains(response, '
  • Answer: Because.
    • ' % a1.pk, html=True) self.assertContains(response, '
  • Answer: Yes.
    • ' % a2.pk, html=True) + def test_model_admin_default_delete_action_no_change_url(self): + """ + Default delete action shouldn't break if a user's ModelAdmin removes the url for change_view. + + Regression test for #20640 + """ + obj = UnchangeableObject.objects.create() + action_data = { + ACTION_CHECKBOX_NAME: obj.pk, + "action": "delete_selected", + "index": "0", + } + response = self.client.post('/test_admin/admin/admin_views/unchangeableobject/', action_data) + # No 500 caused by NoReverseMatch + self.assertEqual(response.status_code, 200) + # The page shouldn't display a link to the nonexistent change page + self.assertContains(response, "
  • Unchangeable object: UnchangeableObject object
    • ", 1, html=True) + def test_custom_function_mail_action(self): "Tests a custom action defined in a function" action_data = {