Opened 10 years ago

Last modified 4 weeks ago

#2856 new Bug

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

Reported by: anonymous Owned by:
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
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 9 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 9 years ago.
Add myself to AUTHORS.
t2856.diff (1.5 KB) - added by Dennis Kaarsemaker 7 years ago.
Backwards compatible patch

Download all attachments as: .zip

Change History (26)

comment:1 Changed 10 years ago by Simon G. <dev@…>

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 Changed 9 years ago by anonymous

Owner: changed from nobody to anonymous
Status: newassigned

comment:3 Changed 9 years ago by ionut bizau

Owner: changed from anonymous to ionut bizau
Status: assignednew

Changed 9 years ago by ionut bizau

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 Changed 9 years ago by ionut bizau

Has patch: set
Owner: ionut bizau deleted

Changed 9 years ago by ionut bizau

Attachment: authors.diff added

Add myself to AUTHORS.

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

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....

comment:6 in reply to:  5 Changed 9 years ago by ionut bizau

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 Changed 8 years ago by Karen Tracey

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.

Changed 7 years ago by Dennis Kaarsemaker

Attachment: t2856.diff added

Backwards compatible patch

comment:8 Changed 7 years ago by Dennis Kaarsemaker

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 Changed 6 years ago by DmitriyShini

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

comment:10 Changed 6 years ago by Łukasz Rekucki

Severity: normalNormal
Type: defectBug

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 Changed 4 years ago by anonymous

Owner: set to anonymous
Status: newassigned

comment:14 Changed 4 years ago by lpiatek

Owner: changed from anonymous to lpiatek

comment:15 Changed 4 years ago by lpiatek

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

comment:16 Changed 4 years ago by Wiktor

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

Pull request looks good, assertions provided too.

comment:17 Changed 4 years ago by Marc Tamlyn

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 Changed 4 years ago by Tim Graham

Patch needs improvement: set

comment:19 Changed 4 weeks ago by Karen Tracey

Owner: changed from lpiatek to Karen Tracey

comment:20 Changed 4 weeks ago by Karen Tracey

Owner: Karen Tracey deleted
Status: assignednew

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

comment:21 Changed 4 weeks ago by Karen Tracey

Patch needs improvement: unset

comment:22 Changed 4 weeks ago by Carl Meyer

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 Changed 4 weeks ago by Karen Tracey

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.

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