Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#15931 closed Cleanup/optimization (invalid)

Patch for admin change form to dodge multi-db bug

Reported by: David Chandek-Stark Owned by: nobody
Component: contrib.admin Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX:

Description

A multi-db bug in django.contrib.contenttypes is triggered via the "View on site" link on the admin site change form when the model uses a different db from the contenttypes app (see discussion at https://groups.google.com/d/topic/django-users/Y50SimNd8zo/discussion and related ticket at #15610). If I follow the logic correctly, there is no need to route the "View on site" link through django.contrib.contenttypes.views.shortcut b/c the admin view has already determined that the model has implemented get_absolute_url, and the shortcut view simply repeats the process -- i.e. by a convoluted method that gets the object and calls get_absolute_url -- except that it trips the aforementioned bug. While this bug should obviously be fixed, the change form can be patched with the attached file.

Attachments (1)

admin__change_form.html.diff (685 bytes) - added by David Chandek-Stark 6 years ago.

Download all attachments as: .zip

Change History (4)

Changed 6 years ago by David Chandek-Stark

comment:1 Changed 6 years ago by Aymeric Augustin

Has patch: set
Needs tests: set
Triage Stage: UnreviewedDesign decision needed
Type: UncategorizedCleanup/optimization

This line has been there forever — at least since adrian merged the new-admin branch at r1434. I don't know why it doesn't use get_absolute_url directly.

The core devs usually prefer fixing the root cause rather working around bugs, so I'll mark this as DDN.

A minor note: your patch is not relative to the "trunk" directory, which is the usual convention for patches here.

comment:2 Changed 6 years ago by Karen Tracey

Resolution: invalid
Status: newclosed

You seem to have ignored all of the logic in the shortcut view that attempts to determine the correct site to redirect to, starting here: http://code.djangoproject.com/browser/django/trunk/django/contrib/contenttypes/views.py#L28

On the face of it, then, it doesn't look to me like replacing a call to the shortcut view with a call to get_absolute_url directly will be equivalent in all cases. Thus attempting to dodge one bug by making this substitution in admin code is not the right thing to do, even as a stopgap measure: it will introduce a regression for anyone relying on that site-choosing logic being in place. The right way to approach this is to fix the base bug.

comment:3 in reply to:  2 Changed 6 years ago by David Chandek-Stark

Replying to kmtracey:

You seem to have ignored all of the logic in the shortcut view that attempts to determine the correct site to redirect to, starting here: http://code.djangoproject.com/browser/django/trunk/django/contrib/contenttypes/views.py#L28

Yes, sorry, good catch. In the case at hand I was dealing with a fully-qualified URL. I suppose the admin view or template could check for this as well, but hopefully a fix of the base bug will available soon.

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