Opened 6 years ago

Closed 8 months ago

Last modified 8 months ago

#11775 closed New feature (fixed)

ABSOLUTE_URL_OVERRIDES doesn't work for the majority of contrib models

Reported by: jukvalim Owned by: jukvalim
Component: Database layer (models, ORM) Version: master
Severity: Release blocker Keywords:
Cc: jukvalim 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 jukvalim 6 years ago.
Proposed patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by jukvalim

  • Status changed from new to assigned

comment:2 Changed 6 years ago by russellm

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

Proposed patch

comment:3 Changed 6 years ago by jukvalim

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

comment:4 Changed 6 years ago by jukvalim

  • Cc jukvalim added

comment:5 Changed 6 years ago by jukvalim

  • Needs tests unset
  • Patch needs improvement unset

comment:6 Changed 4 years ago by baumer1122

  • Severity set to Normal
  • Type set to New feature

comment:7 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

patch.diff fails to apply cleanly on to trunk

comment:8 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 11 months ago by aaugustin

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

comment:10 Changed 10 months ago by timo

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 8 months ago by timgraham

  • Patch needs improvement unset
  • Severity changed from Normal to Release 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 8 months ago by prestontimmons

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 8 months ago by prestontimmons

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 8 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 8 months 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