Opened 11 years ago

Closed 11 years ago

#19402 closed Cleanup/optimization (fixed)

django.contrib.auth.models.CustomUser.REQUIRED_FIELDS needs better docs

Reported by: Daniel Greenfeld Owned by: nobody
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Attachments (2)

19402.diff (1.0 KB ) - added by Preston Holmes 11 years ago.
19402.2.diff (1.3 KB ) - added by Tim Graham 11 years ago.

Download all attachments as: .zip

Change History (12)

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

Resolution: needsinfo
Status: newclosed

I'm not disputing that this might be confusing, but this ticket doesn't serve as an effective call to action because it doesn't clarify what the source of confusion *is*. "Make it more better" doesn't help.

You have to list all the fields that are required. The example does this. I'm happy to oblige with an update to docs, but I need to hear a little more about what exactly is confusing here.

comment:2 by Daniel Greenfeld, 11 years ago

I completely understand the need for the REQUIRED_FIELDS feature. I just wanted to have it documented before people raised issues of the same sort as what we got with CBVs. In other words, I was trying to step up to back up your decision on the code.

I'll figure out a better way to articulate the need for this ticket, and the documentation, and more, and then I'll open it again.

comment:3 by Mattias Linnap, 11 years ago

A link to a discussion that potentially inspired this ticket: http://news.ycombinator.com/item?id=4855590

In summary, django models already have ways of marking fields required via null=False or blank=True on the database and form-level validation. How do these interact with the REQUIRED_FIELDS list?

At the moment, REQUIRED_FIELDS appears to be read only by contrib/auth/management/commands/createsuperuser.py.
It is validated to not contain the USERNAME_FIELD in core/management/validation.py.

Some sources of confusion that may be helpful to document (or, if they should never happen, perhaps additional validation):

  • What are the use cases where REQUIRED_FIELDS differs from fields marked required with null=False or blank=True?
  • I assume user creation will fail if REQUIRED_FIELDS omits fields which are required on the database level - should this be validated?
  • What parts of django might check REQUIRED_FIELDS in the future? Will ModelForms or ModelAdmins read it and reject creating users with missing fields, or is that up to the user code to validate?
  • It's possible to bypass various levels of validation. For example, an user object could be created by createsuperuser.py, by a ModelForm, by a custom form, in the admin site, by code that creates and saves an instance of the swapped User model, or in the database directly. It's possible to load and read users after they have been created in various ways. Which of these will fail currently, or should fail in future versions due to entries in REQUIRED_FIELDS?
Last edited 11 years ago by Mattias Linnap (previous) (diff)

comment:4 by Preston Holmes, 11 years ago

Resolution: needsinfo
Status: closedreopened
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 1.5-beta-1master

Mattias makes some good points.

I think it is well worth documenting the differences between user REQUIRED_FIELDS vs field required param in the context of the custom user docs.

When the username check was added, there was acknowledgement that this was a practical>purity move given that there should be a generic way to handle model validation.

Ideally I'd like to say that anyone adding a custom user should be able to see this for what it is, given that adding a custom user to a project will add some extra work or expertise requirement - but that isn't a good reason not to add some clarification.

by Preston Holmes, 11 years ago

Attachment: 19402.diff added

comment:5 by Preston Holmes, 11 years ago

I've attached a quick stab at this.

This, currently really only used for the interactive creation of users so that it can prompt for fields before actually creating the user.

There is no check that REQUIRED_FIELDS actually contains all fields on the model that have blank=False/undefined

Furthermore, if there are errors in field validation during createsuperuser, the value is just set to None and sent on to model creation anyway:

https://github.com/django/django/blob/b3b3db3d954a5226f870a0b4403343c78efae8dc/django/contrib/auth/management/commands/createsuperuser.py#L96

https://github.com/django/django/blob/b3b3db3d954a5226f870a0b4403343c78efae8dc/django/contrib/auth/management/commands/createsuperuser.py#L115

Given this current limited use, I think it is worth at least considering renaming this attribute to something like "PROMPTED_FIELDS" - I actually don't think there are other places this attribute should be used outside of interactive/prompted creation (and even that could possibly be done by surveying what fields have blank=False). I think the main point of confusion that this attribute introduces is the idea that for user models, a "required field" is somehow distinct or unusual compared to other Django models - where really what this is about is "lets not try to create a user blindly with trial and error for fields we know you want to ask for".

The more I think about it - the more this attr could probably just go away, and instead be compiled dynamically in createsuper user from those fields where blank=False?

refs #19150, #19067 (and to a lesser degree: #19077, #19079)

comment:6 by Tim Graham, 11 years ago

Has patch: set

comment:7 by Marc Tamlyn, 11 years ago

It should also be mentioned that it fails miserably for FKs

by Tim Graham, 11 years ago

Attachment: 19402.2.diff added

comment:8 by Tim Graham, 11 years ago

I made some minor tweaks to Preston's patch, doesn't seem like we're going to rename this at this point in the 1.5 release cycle?

comment:9 by Russell Keith-Magee, 11 years ago

For my money, a name change for REQUIRED_FIELDS is a bit of a bikeshed. REQUIRED_FIELDS isn't inaccurate -- it describes a list of fields that are required on the model. I'm not convinced that renaming the attribute will mean people understand what it's for any better.

If we're going to address this, lets look into at removing the need for REQUIRED_FIELDS altogether (which is something we should be able to add in a backwards compatible way -- we just stop paying attention to the setting, and maybe raise a warning if it is found). However, this isn't going to happen for 1.5.

comment:10 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: reopenedclosed

In 1702be89c86d1b1a01efb16fa7836335ad709184:

[1.5.X] Fixed #19402 - Clarified purpose of CustomUser.REQUIRED_FIELDS

Thanks pydanny for the report and ptone for the patch.

Backport of 24a2bcbcdd from master

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