Opened 6 years ago

Closed 6 years ago

Last modified 5 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: UI/UX:

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 6 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 6 years ago by Russell Keith-Magee

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

Changed 6 years ago by Gabriel Hurley

comment:2 Changed 6 years ago by Gabriel Hurley

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 Changed 6 years ago by Paul McMillan

Triage Stage: AcceptedReady for checkin

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

comment:4 Changed 6 years ago by Honza Král

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 Changed 6 years ago by Honza Král

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

Backport from trunk (r14251).

comment:6 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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