Opened 7 years ago

Closed 2 years ago

Last modified 2 years ago

#11775 closed New feature (fixed)

ABSOLUTE_URL_OVERRIDES doesn't work for the majority of contrib models

Reported by: Jukka Välimaa Owned by: Jukka Välimaa
Component: Database layer (models, ORM) Version: master
Severity: Release blocker Keywords:
Cc: Jukka Välimaa 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

At the moment, ABSOLUTE_URL_OVERRIDES is used to override get_absolute_url methods. However, most models in django.contrib don't have the get_absolute_url method in the first place, and so ABSOLUTE_URL_OVERRIDES does nothing to them. This means that you can't have links from unmodified admin pages of these model objects to the page that represents the model object in the application. If there is code that relies on getting the url to object by get_absolute_url, there needs to be separate code to handle these model objects.

The reporter of this ticket came across both of these problems when wanting to link to the application page of a group (contrib.auth.models.Group) from admin, and from a general search.

Proposed solution: Make it possible for ABSOLUTE_URL_OVERRIDES to insert a function as get_absolute_url method of a model class, rather than only overriding existing methods. (Design decision needed on this one, I think)

Attachments (1)

patch.diff (4.4 KB) - added by Jukka Välimaa 7 years ago.
Proposed patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by Jukka Välimaa

Status: newassigned

comment:2 Changed 7 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Seems like a reasonable idea to me, and the implementation is relatively simple.

One problem with the patch - it needs tests. The fact that there aren't any existing tests for ABSOLUTE_URL_OVERRIDES doesn't excuse you from writing some. If you want to win extra Brownie points, you should take the opportunity to write a full test suite for the entire ABSOLUTE_URL_OVERRIDES feature, not just the bit you added.

I'm also interested to see if the use of currying can be removed altogether. While currying is a neat trick, it doesn't seem necessary in this case. The historical use is actually more computationally intensive than it should be. All we are doing here is substituting cls.get_absolute_url with the appropriate override. We don't need to re-evaluate ABSOLUTE_URL_OVERRIDES on every call to get_absolute_url.

As prior warning: there is also a lingering background issue surrounding this idea and its intersection with ReplacingGetAbsoluteUrl. No formal decisions have been made, but if a decision *is* made, it will impact on this ticket and patch.

Changed 7 years ago by Jukka Välimaa

Attachment: patch.diff added

Proposed patch

comment:3 Changed 7 years ago by Jukka Välimaa

Added two tests for ABSOLUTE_URL_OVERRIDES feature, removed use of currying.

comment:4 Changed 7 years ago by Jukka Välimaa

Cc: Jukka Välimaa added

comment:5 Changed 7 years ago by Jukka Välimaa

Needs tests: unset
Patch needs improvement: unset

comment:6 Changed 6 years ago by Peter Baumgartner

Severity: Normal
Type: New feature

comment:7 Changed 6 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

patch.diff fails to apply cleanly on to trunk

comment:8 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 Changed 3 years ago by Aymeric Augustin

Can we deprecate ABSOLUTE_URL_OVERRIDES instead? Currently get_absolute_url and related features create lots of cross-layer coupling.

comment:10 Changed 2 years ago by Tim Graham

I was going to suggest we could replace settings.ABSOLUTE_URL_OVERRIDES.get('%s.%s' % (opts.app_label, opts.model_name) with URL reversing; something like reverse('%s.%s-absolute-url') % (opts.app_label, opts.model_name), but this would force people to rename their URLs in order to match the convention.

Given the lack of interest in this ticket, maybe it would be better to simply "won't fix" it and remove it when we come up with a solution for #8264 "Replace get_absolute_url with more sane alternative".

comment:11 Changed 2 years ago by Tim Graham

Patch needs improvement: unset
Severity: NormalRelease blocker

As reported in this PR, Django 1.7 broke the ability to use ABSOLUTE_URL_OVERRIDES for auth.User since it's get_absolute_url() was removed in #20881.

I think we should fix this ticket in order to fix that regression. PR

comment:12 Changed 2 years ago by Preston Timmons

Tim, I reviewed PR. The changes look good and work for me.

I agree, the regression should be fixed for as long as ABSOLUTE_URL_OVERRIDES is part of Django.

comment:13 Changed 2 years ago by Preston Timmons

Triage Stage: AcceptedReady for checkin

comment:14 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In c32bc1a7a7bbb3d5bd0a2c11bc77dd5ab1c32fbc:

Fixed #11775 -- Made ABSOLUTE_URL_OVERRIDES work with models that don't define get_absolute_url().

Thanks jukvalim for the report and initial patch,
and Preston Timmons for review.

comment:15 Changed 2 years ago by Tim Graham <timograham@…>

In a8ded528b3c2f66d56f1f5499cf2021f3829c8e4:

[1.7.x] Fixed #11775 -- Made ABSOLUTE_URL_OVERRIDES work with models that don't define get_absolute_url().

Thanks jukvalim for the report and initial patch,
and Preston Timmons for review.

Backport of c32bc1a7a7 from master

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