Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#15931 closed Cleanup/optimization (invalid)

Patch for admin change form to dodge multi-db bug

Reported by: dchandek 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:


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 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 dchandek 5 years ago.

Download all attachments as: .zip

Change History (4)

Changed 5 years ago by dchandek

comment:1 Changed 5 years ago by aaugustin

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed
  • Type changed from Uncategorized to Cleanup/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 follow-up: Changed 5 years ago by kmtracey

  • Resolution set to invalid
  • Status changed from new to closed

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:

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

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:

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