Code

Opened 8 years ago

Last modified 11 months ago

#2856 assigned Bug

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

Reported by: anonymous Owned by: lpiatek
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 6 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 6 years ago.
Add myself to AUTHORS.
t2856.diff (1.5 KB) - added by seveas 5 years ago.
Backwards compatible patch

Download all attachments as: .zip

Change History (21)

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

  • Summary changed from Page not found 404 for on add and edit occurences of deleted objects listed in Recent Actions to Admin's Recent Actions will link to a 404 when an object has been deleted.
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 0.95 to SVN

confirmed on latest svn.

comment:2 Changed 6 years ago by anonymous

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:3 Changed 6 years ago by ionut_bizau

  • Owner changed from anonymous to ionut_bizau
  • Status changed from assigned to new

Changed 6 years ago by ionut_bizau

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 follow-up: Changed 6 years ago by ionut_bizau

  • Has patch set
  • Owner ionut_bizau deleted

Changed 6 years ago by ionut_bizau

Add myself to AUTHORS.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 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 6 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 5 years ago by kmtracey

  • 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 5 years ago by seveas

Backwards compatible patch

comment:8 Changed 5 years ago by seveas

  • 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 3 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 3 years ago by lrekucki

  • Severity changed from normal to Normal
  • Type changed from defect to Bug

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 11 months ago by anonymous

  • Owner set to anonymous
  • Status changed from new to assigned

comment:14 Changed 11 months ago by lpiatek

  • Owner changed from anonymous to lpiatek

comment:15 Changed 11 months ago by lpiatek

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

comment:16 Changed 11 months ago by viciu

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

Pull request looks good, assertions provided too.

comment:17 Changed 11 months ago by mjtamlyn

  • Triage Stage changed from Ready for checkin to Accepted

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 11 months ago by timo

  • Patch needs improvement set

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from lpiatek to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.