Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13790 closed (fixed)

Docs for CurrentSiteManager indicate an M2M field called "sites" will be auto-detected on a model when it won't

Reported by: gabrielhurley Owned by: gabrielhurley
Component: Contrib apps Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


Throughout the Sites Framework docs the example models use sites = ManyToManyField(Site), and under the CurrentSiteManager section it indicates that CurrentSiteManager will be able to automatically discover either a ForeignKey called "site" or a ManyToManyField called "sites", when in fact the code only looks for a field called "site".

While it'd be an easy docs fix, it seems more valuable to fix the code for the manager to actually do what the docs say.

I'm happy to write up a patch and tests for it later.

Attachments (1)

13790_CurrentSiteManager_patch.diff (6.0 KB) - added by gabrielhurley 5 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 5 years ago by russellm

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

I've got deja vu here... I addressed this documentation in #13168. Looks like the new text is just as misleading as the last lot.

However, agreed that the natural thing is to allow 'sites' as well. My take on the right implementation:

  • if a field_name argument is provided, it is used unconditionally.
  • otherwise, look for a field called 'site'
  • otherwise, look for a field called 'sites'

For bonus points, we could add extra validation protection; the current implementation is very lax, in that it allows *any* field called site (including an integer field, etc).

Changed 5 years ago by gabrielhurley

comment:2 Changed 5 years ago by gabrielhurley

  • Has patch set
  • Owner changed from nobody to gabrielhurley
  • Status changed from new to assigned

Patch added which resolves the issues as follows:

  • Uses a field name passed to the manager above all else
  • If no name is given, it checks for a field named either "site" or "sites"
  • Raises a ValueError if a field with the appropriate name is not found on the model
  • Validates that the field found on the model is either a ForeignKey or ManyToManyField
  • Adds a full set of new unit tests to cover the previously completely untested CurrentSiteManager.

Changes pass the full test suite, I don't foresee any backwards incompatibilities, and the code is now in line with the docs.

If anyone sees refinements for the code feel free to let me know, but this seemed like the most maintainable and extensible way to implement the field name validation.

comment:3 Changed 5 years ago by PaulM

  • Triage Stage changed from Accepted to Ready for checkin

This patch looks good and applies for me. Marking as RFC.

comment:4 Changed 5 years ago by Honza_Kral

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

(In [14251]) Fixed #13790 -- auto detection of m2m fields to Site. Thanks, gabrielhurley!

Tests are placed in the test suite and not the contrib app since they require
models to work

comment:5 Changed 5 years ago by Honza_Kral

(In [14252]) [1.2.X] Fixed #13790 -- auto detection of m2m fields to Site. Thanks, gabrielhurley!

Backport from trunk (r14251).

comment:6 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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