Opened 3 years ago

Last modified 15 months ago

#18148 new New feature

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

Reported by: rgeoghegan Owned by: nobody
Component: contrib.sites Version: 1.4
Severity: Normal Keywords:
Cc: sfllaw@…, krzysiumed@…, rgeoghegan 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 rgeoghegan 3 years ago.
Patch for django-trunk revision 17913
sites.2.diff (8.7 KB) - added by rgeoghegan 3 years ago.
Improved patch after comments from krzysiumed
18148_v3.diff (8.7 KB) - added by krzysiumed 3 years ago.

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by rgeoghegan

Patch for django-trunk revision 17913

comment:1 Changed 3 years ago by sfllaw

  • Cc sfllaw@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by jezdez

  • Component changed from Uncategorized to contrib.sites
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to New feature

comment:3 Changed 3 years ago by krzysiumed

  • 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 3 years ago by krzysiumed

  • Cc krzysiumed@… added

comment:5 Changed 3 years ago by rgeoghegan

  • Cc rgeoghegan added

Changed 3 years ago by rgeoghegan

Improved patch after comments from krzysiumed

comment:6 Changed 3 years ago by rgeoghegan

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

Changed 3 years ago by krzysiumed

comment:7 Changed 3 years ago by krzysiumed

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 3 years ago by krzysiumed

  • Patch needs improvement unset

comment:9 Changed 15 months ago by timo

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