Opened 5 years ago

Last modified 2 years ago

#18148 new New feature

django.contrib.sites.managers.CurrentSiteManager should be able to span multiple models

Reported by: Rory Geoghegan Owned by: nobody
Component: contrib.sites Version: 1.4
Severity: Normal Keywords:
Cc: sfllaw@…, krzysiumed@…, Rory Geoghegan Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The current implementation of CurrentSiteManager can only be used with models that have a ForeignKey or ManyToManyField reference to a Site model. Because get_query_set uses the built-in filter syntax, you could specify remote model references by using a double underscore, except that the validation code breaks.

For example, you should be able to do:

class Foo(model.Model):
   bar = models.ForeignKey('Bar')
   on_site = CurrentSiteManager('bar__site')

and it should do the right thing.

Attachments (3)

sites.diff (7.2 KB) - added by Rory Geoghegan 5 years ago.
Patch for django-trunk revision 17913
sites.2.diff (8.7 KB) - added by Rory Geoghegan 5 years ago.
Improved patch after comments from krzysiumed
18148_v3.diff (8.7 KB) - added by Christopher Medrela 4 years ago.

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by Rory Geoghegan

Attachment: sites.diff added

Patch for django-trunk revision 17913

comment:1 Changed 5 years ago by Simon Law

Cc: sfllaw@… added

comment:2 Changed 5 years ago by Jannis Leidel

Component: Uncategorizedcontrib.sites
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

comment:3 Changed 5 years ago by Christopher Medrela

Patch needs improvement: set

The patch looks really well, but some error messages could be better:

If ArticleComment model (from regressiontests.sites_framework.models module) is changed in this way:

    on_site = CurrentSiteManager("parent_article__non_existing_field")

then the error message is

ValueError: CurrentSiteManager couldn't find a field named parent_article__non_existing_field in ExclusiveArticle.

The field name is non_existing_field, but the message says it's parent_article__non_existing_field.

The other situation when the error message is not meaningful enough is when the field exists but it's neither ForeignKey nor ManyToManyField. See example:

TypeError: invalid_field must be a ForeignKey or ManyToManyField.

The message doesn't say name of the model where invalid_field was defined.

I also suggest using first_article and second_article instead of article and article2. It's not more meaningful, but it's easier to see the difference.

comment:4 Changed 5 years ago by Christopher Medrela

Cc: krzysiumed@… added

comment:5 Changed 5 years ago by Rory Geoghegan

Cc: Rory Geoghegan added

Changed 5 years ago by Rory Geoghegan

Attachment: sites.2.diff added

Improved patch after comments from krzysiumed

comment:6 Changed 5 years ago by Rory Geoghegan

@krzysiumed: I updated the patch after your comments with the changes you requested.

Changed 4 years ago by Christopher Medrela

Attachment: 18148_v3.diff added

comment:7 Changed 4 years ago by Christopher Medrela

The patch seems to be ready to check in, but I would like to suggest
some ideas. I attached patch including them.

The documentation is slightly unclear -- it's not obvious to which
paragraph refers the 'Changed in Django 1.5'. I added subsections
which make it less ambiguous, but the titles (especially 'Spanning relationship')
could be better. I also changed the section to which
the only link points. The previous target was 'Field lookups' section
which describes lte (e. g. pub_date__lte), exact
(e. g. headline__exact) and other features. I think that 'Lookups that span relationships'
section is more appropriate. Finally, I
would be happy if the patch were reviewed by a native speaker.

The changes in code (and tests) are usually renamings and following
PEP8. I renamed first_article and second_article to
article_inside and article_outside (I did the same about comment
and comment2). The idea is to make the names more meaningful but I'm
not sure if the goal was achieved.

These ideas are propositions so feel free to reject them if you want.

comment:8 Changed 4 years ago by Christopher Medrela

Patch needs improvement: unset

comment:9 Changed 2 years ago by Tim Graham

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top