Opened 14 years ago

Closed 13 years ago

Last modified 12 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: Gabriel Hurley Owned by: Gabriel Hurley
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: no UI/UX: no

Description

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 Gabriel Hurley 14 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

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).

by Gabriel Hurley, 14 years ago

comment:2 by Gabriel Hurley, 14 years ago

Has patch: set
Owner: changed from nobody to Gabriel Hurley
Status: newassigned

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 by Paul McMillan, 13 years ago

Triage Stage: AcceptedReady for checkin

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

comment:4 by Honza Král, 13 years ago

Resolution: fixed
Status: assignedclosed

(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 by Honza Král, 13 years ago

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

Backport from trunk (r14251).

comment:6 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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