Opened 17 years ago

Closed 7 years ago

Last modified 7 years ago

#2856 closed Bug (fixed)

Admin's Recent Actions will link to a 404 when an object has been deleted.

Reported by: anonymous Owned by: Tim Graham <timograham@…>
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A page not found (404) is begin show when clicking on an add or edit action of a (later) deleted object in Recent Actions.

For the delete action, there is already no link on the object. But the older add and edit actions in the Recent Actions list have at this time no valid URL beacause the object is already deleted.

Because this list is in chronological order, one could disable the links on the add and edit actions in Recent Actions that have taken place earlier that the delete. With again allowing a link on a later than the delete created new object with another add actin.

This change would make de admin page more stable since this will also be used with less technical personel and will ensure that the Recent Actions has only valid (non 404) URLs.

Attachments (3)

2856.diff (3.2 KB ) - added by ionut bizau 16 years ago.
Added a new field (object_exists) to LogEntry, which becomes False after the object that that entry is referring to is deleted. I think this approach is better than just looking for the object by id, both for performance and for the situation where an object with the same id is added later. This field is then checked in the template.
authors.diff (315 bytes ) - added by ionut bizau 16 years ago.
Add myself to AUTHORS.
t2856.diff (1.5 KB ) - added by Dennis Kaarsemaker 15 years ago.
Backwards compatible patch

Download all attachments as: .zip

Change History (29)

comment:1 by Simon G. <dev@…>, 17 years ago

Summary: Page not found 404 for on add and edit occurences of deleted objects listed in Recent ActionsAdmin's Recent Actions will link to a 404 when an object has been deleted.
Triage Stage: UnreviewedAccepted
Version: 0.95SVN

confirmed on latest svn.

comment:2 by anonymous, 16 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:3 by ionut bizau, 16 years ago

Owner: changed from anonymous to ionut bizau
Status: assignednew

by ionut bizau, 16 years ago

Attachment: 2856.diff added

Added a new field (object_exists) to LogEntry, which becomes False after the object that that entry is referring to is deleted. I think this approach is better than just looking for the object by id, both for performance and for the situation where an object with the same id is added later. This field is then checked in the template.

comment:4 by ionut bizau, 16 years ago

Has patch: set
Owner: ionut bizau removed

by ionut bizau, 16 years ago

Attachment: authors.diff added

Add myself to AUTHORS.

in reply to:  4 ; comment:5 by Karen Tracey <kmtracey@…>, 16 years ago

Replying to:

Added a new field (object_exists) to LogEntry?, which becomes False after the object that that entry is referring to is deleted. I think this approach is better than just >looking for the object by id, both for performance and for the situation where an object with the same id is added later. This field is then checked in the template.

Isn't this a major backwards-incompatible change? My existing DB's django_admin_log table doesn't have this new field. I don't expect when I pull a new svn checkout that I have to resync my db tables to accommodate changes like this....

in reply to:  5 comment:6 by ionut bizau, 16 years ago

Replying to Karen Tracey <kmtracey@gmail.com>:

Isn't this a major backwards-incompatible change? My existing DB's django_admin_log table doesn't have this new field. I don't expect when I pull a new svn checkout that I have to resync my db tables to accommodate changes like this....

Being before 1.0 I guess we can break compatibility, but I am not sure to what degree. Let's wait for input from a core member.

comment:7 by Karen Tracey, 15 years ago

Patch needs improvement: set

I don't believe this problem warrants the backwards-incompatible change of modifying the admin table schema. We're also past 1.0 now so if this is to be fixed in the 1.x line a backwards-compatible way of doing it needs to be developed.

by Dennis Kaarsemaker, 15 years ago

Attachment: t2856.diff added

Backwards compatible patch

comment:8 by Dennis Kaarsemaker, 15 years ago

Needs tests: set

Here is a first stab at a backwards compatible patch. It's rather heavy as it requires at least one SQL query per displayed log entry so I'm not sure the approach is the best. It also needs tests for the added function.

comment:9 by DmitriyShini, 13 years ago

I used this patch for Django v1.3 alha. Now it's worked, but it removes a functional.

comment:10 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Type: defectBug

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by anonymous, 11 years ago

Owner: set to anonymous
Status: newassigned

comment:14 by lpiatek, 11 years ago

Owner: changed from anonymous to lpiatek

comment:15 by lpiatek, 11 years ago

Pull request/patch for 1.6 is here https://github.com/django/django/pull/1144

comment:16 by Wiktor, 11 years ago

Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Pull request looks good, assertions provided too.

comment:17 by Marc Tamlyn, 11 years ago

Triage Stage: Ready for checkinAccepted

Hmm, I don't like this approach. An alternative could be to insert an additional redirect view which will check whether the object exists, and if not redirect to the root of the admin with a redirect.

Another wider reaching alternative would be to generally improve 404 handling in the admin so it gives nicer messages and redirects.

comment:18 by Tim Graham, 11 years ago

Patch needs improvement: set

comment:19 by Karen Tracey, 7 years ago

Owner: changed from lpiatek to Karen Tracey

comment:20 by Karen Tracey, 7 years ago

Owner: Karen Tracey removed
Status: assignednew

Tried to address Marc's concerns in new PR https://github.com/django/django/pull/7495

comment:21 by Karen Tracey, 7 years ago

Patch needs improvement: unset

comment:22 by Carl Meyer, 7 years ago

Is this really even a bug? Seems to me that if this link is supposed to go to the admin page for the object, and the object no longer exists, a 404 is a pretty reasonable response. Do we just need a nicer default 404 page in the admin?

comment:23 by Karen Tracey, 7 years ago

I do believe it is confusing for general users for the admin to be offering links to pages which then 404. It was accepted 10 years ago and seemed reasonable to me. Not all admin users are sophisticated users who immediately jump from 404 to "oh that thing must be gone", rather they get the impression from the "broken links" that the admin is broken/unreliable somehow. I likely would have first gone for the "just don't make it a link in recent actions" approach but given that had been rejected already I tried an alternative way. Running DEBUG mode the reason for the 404 is clear since the Http404 messages are visible then to the user, but those are not visible when DEBUG is off, so I though making them a message the user would see would be a small improvement. I'm not particularly willing to tackle improving the 404 page of admin in general.

comment:24 by Tim Graham, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:25 by Tim Graham <timograham@…>, 7 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 27267afc:

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.

comment:26 by Tim Graham <timograham@…>, 7 years ago

In 8ea541e:

Refs #2856 -- Removed redundant escaping in admin's "Perhaps it was deleted?" message.

Note: See TracTickets for help on using tickets.
Back to Top