Fixed #2856 -- Replaced some 404s with messages in admin.

Instead of a 404, return a redirect to admin index page with a message
indicating that the requested object does not exist. This avoids the
admin returning 404 from "Recent Actions" links for deleted objects.
This commit is contained in:
Karen Tracey 2016-11-05 09:56:03 -04:00 committed by Tim Graham
parent 946dd5bde2
commit 27267afc41
2 changed files with 57 additions and 25 deletions

View File

@ -34,7 +34,7 @@ from django.forms.models import (
modelform_factory, modelformset_factory, modelform_factory, modelformset_factory,
) )
from django.forms.widgets import CheckboxSelectMultiple, SelectMultiple from django.forms.widgets import CheckboxSelectMultiple, SelectMultiple
from django.http import Http404, HttpResponseRedirect from django.http import HttpResponseRedirect
from django.http.response import HttpResponseBase from django.http.response import HttpResponseBase
from django.template.response import SimpleTemplateResponse, TemplateResponse from django.template.response import SimpleTemplateResponse, TemplateResponse
from django.urls import reverse from django.urls import reverse
@ -1389,6 +1389,19 @@ class ModelAdmin(BaseModelAdmin):
initial[k] = initial[k].split(",") initial[k] = initial[k].split(",")
return initial return initial
def _get_obj_does_not_exist_redirect(self, request, opts, object_id):
"""
Create a message informing the user that the object doesn't exist
and return a redirect to the admin index page.
"""
msg = _("%(name)s with ID %(key)s doesn't exist. Perhaps it was deleted?") % {
'name': force_text(opts.verbose_name),
'key': escape(object_id),
}
self.message_user(request, msg, messages.WARNING)
url = reverse('admin:index', current_app=self.admin_site.name)
return HttpResponseRedirect(url)
@csrf_protect_m @csrf_protect_m
def changeform_view(self, request, object_id=None, form_url='', extra_context=None): def changeform_view(self, request, object_id=None, form_url='', extra_context=None):
with transaction.atomic(using=router.db_for_write(self.model)): with transaction.atomic(using=router.db_for_write(self.model)):
@ -1419,8 +1432,7 @@ class ModelAdmin(BaseModelAdmin):
raise PermissionDenied raise PermissionDenied
if obj is None: if obj is None:
raise Http404(_('%(name)s object with primary key %(key)r does not exist.') % { return self._get_obj_does_not_exist_redirect(request, opts, object_id)
'name': force_text(opts.verbose_name), 'key': escape(object_id)})
ModelForm = self.get_form(request, obj) ModelForm = self.get_form(request, obj)
if request.method == 'POST': if request.method == 'POST':
@ -1692,10 +1704,7 @@ class ModelAdmin(BaseModelAdmin):
raise PermissionDenied raise PermissionDenied
if obj is None: if obj is None:
raise Http404( return self._get_obj_does_not_exist_redirect(request, opts, object_id)
_('%(name)s object with primary key %(key)r does not exist.') %
{'name': force_text(opts.verbose_name), 'key': escape(object_id)}
)
using = router.db_for_write(self.model) using = router.db_for_write(self.model)
@ -1749,10 +1758,7 @@ class ModelAdmin(BaseModelAdmin):
model = self.model model = self.model
obj = self.get_object(request, unquote(object_id)) obj = self.get_object(request, unquote(object_id))
if obj is None: if obj is None:
raise Http404(_('%(name)s object with primary key %(key)r does not exist.') % { return self._get_obj_does_not_exist_redirect(request, model._meta, object_id)
'name': force_text(model._meta.verbose_name),
'key': escape(object_id),
})
if not self.has_change_permission(request, obj): if not self.has_change_permission(request, obj):
raise PermissionDenied raise PermissionDenied

View File

@ -245,12 +245,16 @@ class AdminViewBasicTest(AdminViewBasicTestCase):
def test_basic_edit_GET_string_PK(self): def test_basic_edit_GET_string_PK(self):
""" """
Ensure GET on the change_view works (returns an HTTP 404 error, see GET on the change_view (when passing a string as the PK argument for a
#11191) when passing a string as the PK argument for a model with an model with an integer PK field) redirects to the index page with a
integer PK field. message saying the object doesn't exist.
""" """
response = self.client.get(reverse('admin:admin_views_section_change', args=('abc',))) response = self.client.get(reverse('admin:admin_views_section_change', args=('abc',)), follow=True)
self.assertEqual(response.status_code, 404) self.assertRedirects(response, reverse('admin:index'))
self.assertEqual(
[m.message for m in response.context['messages']],
["section with ID abc doesn't exist. Perhaps it was deleted?"]
)
def test_basic_edit_GET_old_url_redirect(self): def test_basic_edit_GET_old_url_redirect(self):
""" """
@ -263,12 +267,15 @@ class AdminViewBasicTest(AdminViewBasicTestCase):
def test_basic_inheritance_GET_string_PK(self): def test_basic_inheritance_GET_string_PK(self):
""" """
Ensure GET on the change_view works on inherited models (returns an GET on the change_view (for inherited models) redirects to the index
HTTP 404 error, see #19951) when passing a string as the PK argument page with a message saying the object doesn't exist.
for a model with an integer PK field.
""" """
response = self.client.get(reverse('admin:admin_views_supervillain_change', args=('abc',))) response = self.client.get(reverse('admin:admin_views_supervillain_change', args=('abc',)), follow=True)
self.assertEqual(response.status_code, 404) self.assertRedirects(response, reverse('admin:index'))
self.assertEqual(
[m.message for m in response.context['messages']],
["super villain with ID abc doesn't exist. Perhaps it was deleted?"]
)
def test_basic_add_POST(self): def test_basic_add_POST(self):
""" """
@ -1787,6 +1794,16 @@ class AdminViewPermissionsTest(TestCase):
logged = LogEntry.objects.get(content_type=article_ct, action_flag=DELETION) logged = LogEntry.objects.get(content_type=article_ct, action_flag=DELETION)
self.assertEqual(logged.object_id, str(self.a1.pk)) self.assertEqual(logged.object_id, str(self.a1.pk))
def test_delete_view_nonexistent_obj(self):
self.client.force_login(self.deleteuser)
url = reverse('admin:admin_views_article_delete', args=('nonexistent',))
response = self.client.get(url, follow=True)
self.assertRedirects(response, reverse('admin:index'))
self.assertEqual(
[m.message for m in response.context['messages']],
["article with ID nonexistent doesn't exist. Perhaps it was deleted?"]
)
def test_history_view(self): def test_history_view(self):
"""History view should restrict access.""" """History view should restrict access."""
# add user should not be able to view the list of article or change any of them # add user should not be able to view the list of article or change any of them
@ -1828,8 +1845,12 @@ class AdminViewPermissionsTest(TestCase):
def test_history_view_bad_url(self): def test_history_view_bad_url(self):
self.client.force_login(self.changeuser) self.client.force_login(self.changeuser)
response = self.client.get(reverse('admin:admin_views_article_history', args=('foo',))) response = self.client.get(reverse('admin:admin_views_article_history', args=('foo',)), follow=True)
self.assertEqual(response.status_code, 404) self.assertRedirects(response, reverse('admin:index'))
self.assertEqual(
[m.message for m in response.context['messages']],
["article with ID foo doesn't exist. Perhaps it was deleted?"]
)
def test_conditionally_show_add_section_link(self): def test_conditionally_show_add_section_link(self):
""" """
@ -3609,11 +3630,16 @@ class AdminCustomQuerysetTest(TestCase):
def test_change_view(self): def test_change_view(self):
for i in self.pks: for i in self.pks:
response = self.client.get(reverse('admin:admin_views_emptymodel_change', args=(i,))) url = reverse('admin:admin_views_emptymodel_change', args=(i,))
response = self.client.get(url, follow=True)
if i > 1: if i > 1:
self.assertEqual(response.status_code, 200) self.assertEqual(response.status_code, 200)
else: else:
self.assertEqual(response.status_code, 404) self.assertRedirects(response, reverse('admin:index'))
self.assertEqual(
[m.message for m in response.context['messages']],
["empty model with ID 1 doesn't exist. Perhaps it was deleted?"]
)
def test_add_model_modeladmin_defer_qs(self): def test_add_model_modeladmin_defer_qs(self):
# Test for #14529. defer() is used in ModelAdmin.get_queryset() # Test for #14529. defer() is used in ModelAdmin.get_queryset()