Opened 12 years ago
Closed 9 years ago
#19353 closed New feature (fixed)
Make it easier to extend UserCreationForm for custom user models
Reported by: | Baptiste Mispelon | Owned by: | Berker Peksag |
---|---|---|---|
Component: | contrib.auth | Version: | 1.5-alpha-1 |
Severity: | Normal | Keywords: | UserCreationForm AUTH_USER_MODEL |
Cc: | Russell Keith-Magee, aenor.realm@…, Mjumbe Poe, victorhooi@…, internet@…, flisky, maestrofjp, berker.peksag@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The documentation states that UserCreationForm
must be re-written when using a custom user model.
However, for simple subclasses of AbstractUser
it's actually simpler to extend (rather than rewrite) the existing form like so:
class CustomUserCreationForm(UserCreationForm): class Meta(UserCreationForm.Meta): model = CustomUser
Unfortunately, this fails because the clean_username
method of UserCreationForm
still references the original User
model directly (this technique works with UserChangeForm
though).
To get it working, the original clean_username
method needs to be copied over and modified to use the custom user model, like so:
class CustomUserCreationForm(UserCreationForm): class Meta(UserCreationForm.Meta): model = CustomUser def clean_username(self): username = self.cleaned_data["username"] try: CustomUser.objects.get(username=username) except CustomUser.DoesNotExist: return username raise forms.ValidationError(self.error_messages['duplicate_username'])
This works, but having to copy/paste some code is not a very good practice.
Therefore, I propose to change UserCreationForm.clean_username
to use Meta.model
instead of User
.
This allows the creation of custom user creation forms by simply redefining the user model in the form's Meta
class (like in the first example).
The documentation could also be extended to show an example of how to extend the builtin auth forms that require it.
Attachments (1)
Change History (24)
comment:1 by , 12 years ago
Has patch: | set |
---|
comment:2 by , 12 years ago
Needs tests: | set |
---|
comment:3 by , 12 years ago
Needs tests: | unset |
---|
comment:4 by , 12 years ago
Needs documentation: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 12 years ago
Attachment: | ticket-19353.diff added |
---|
follow-up: 7 comment:5 by , 12 years ago
I've updated the patch with some documentation.
I've also started a branch on my fork: https://github.com/bmispelon/django/tree/ticket-19353
The wording on the documentation feels a bit awkward and I'd appreciate comments and ideas to improve it.
comment:6 by , 12 years ago
Cc: | added |
---|
On a related note, i've added some fields to my subclass of AbstractUser and admin fieldset setting now complains about missing fields from the [change] form. The fix is to use get_user_model() in Meta too.
comment:7 by , 12 years ago
Replying to bmispelon:
I've updated the patch with some documentation.
I've also started a branch on my fork: https://github.com/bmispelon/django/tree/ticket-19353
The wording on the documentation feels a bit awkward and I'd appreciate comments and ideas to improve it.
I just made a similar fix and right before sending a pull request I found your ticket :)
Could submit a pull request?
Also, I left a comment on your branch about the username field: https://github.com/bmispelon/django/commit/81c9bd6a2a55a8cf1c2bee351db873ea7403db1e#commitcomment-2507426
comment:8 by , 11 years ago
Cc: | added |
---|
comment:9 by , 11 years ago
Cc: | added |
---|
Hi,
Curious - what is the status on this ticket?
Are there still blockers on it being accepted?
Cheers,
Victor
comment:10 by , 11 years ago
I ran into this problem today, and was very confused (the stack trace you get when this goes wrong doesn't lead you quickly to where the problem occurred). The patch looks reasonable to me - it's got tests, docs, and a simple enough implementation. Is there anything we can do to get it committed? I'm more than happy to do any additional work here if it'd help, if Baptiste doesn't have time to work on it at the moment.
comment:11 by , 11 years ago
Cc: | added |
---|
comment:12 by , 11 years ago
Cc: | added |
---|
comment:13 by , 10 years ago
Cc: | added |
---|
comment:14 by , 10 years ago
This should be easier after removing some of the logic from the form in #13147.
comment:16 by , 9 years ago
Since 849538d03df21b69f0754a38ee4ec5f48fa02c52 has been committed, I think UserCreationForm
works well with subclasses of AbstractUser
. Here is a test case:
diff --git a/tests/auth_tests/test_forms.py b/tests/auth_tests/test_forms.py index aa0a6af..68a1b88 100644 --- a/tests/auth_tests/test_forms.py +++ b/tests/auth_tests/test_forms.py @@ -10,6 +10,7 @@ from django.contrib.auth.forms import ( SetPasswordForm, UserChangeForm, UserCreationForm, ) from django.contrib.auth.models import User +from django.contrib.auth.tests.custom_user import ExtensionUser from django.contrib.sites.models import Site from django.core import mail from django.core.mail import EmailMultiAlternatives @@ -153,6 +154,20 @@ class UserCreationFormTest(TestDataMixin, TestCase): form['password2'].errors ) + def test_custom_form(self): + class CustomUserCreationForm(UserCreationForm): + class Meta(UserCreationForm.Meta): + model = ExtensionUser + fields = UserCreationForm.Meta.fields + ('date_of_birth',) + + data = { + 'username': 'testclient', + 'password1': 'testclient', + 'password2': 'testclient', + 'date_of_birth': '1988-02-24', + } + form = CustomUserCreationForm(data) + self.assertTrue(form.is_valid()) @override_settings(USE_TZ=False, PASSWORD_HASHERS=['django.contrib.auth.hashers.SHA1PasswordHasher']) class AuthenticationFormTest(TestDataMixin, TestCase):
I can send the test as a PR, if it looks reasonable.
comment:17 by , 9 years ago
Cc: | added |
---|
comment:18 by , 9 years ago
Sure, might want to also consider including some documentation updates from bmispelon's branch in comment 5.
comment:19 by , 9 years ago
I'm also curious to see if UserChangeForm
can now also be used.
FYI, the documentation moved to https://docs.djangoproject.com/en/dev/topics/auth/customizing/#custom-users-and-the-built-in-auth-forms since the original ticket was opened.
Thanks.
comment:20 by , 9 years ago
Needs documentation: | unset |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Pull request: https://github.com/django/django/pull/6142
- Converted my test in comment 16 to an actual test case
- Added a test for UserChangeForm
- Ported documentation updates from bmispelon's branch in comment 5
comment:23 by , 9 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
If there are any further improvements, let's open a new ticket. Thanks!
Seems like a reasonable idea to me. Patch needs a documentation update to point out the changed constraints on form usage.