Opened 4 years ago

Closed 10 months 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)

ticket-19353.diff (5.0 KB) - added by Baptiste Mispelon 4 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by Baptiste Mispelon

Has patch: set

comment:2 Changed 4 years ago by Aymeric Augustin

Needs tests: set

comment:3 Changed 4 years ago by Baptiste Mispelon

Needs tests: unset

comment:4 Changed 4 years ago by Russell Keith-Magee

Needs documentation: set
Triage Stage: UnreviewedAccepted

Seems like a reasonable idea to me. Patch needs a documentation update to point out the changed constraints on form usage.

Changed 4 years ago by Baptiste Mispelon

Attachment: ticket-19353.diff added

comment:5 Changed 4 years ago by Baptiste Mispelon

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 Changed 4 years ago by wiz

Cc: aenor.realm@… 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 in reply to:  5 Changed 4 years ago by rpedigoni

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 Changed 3 years ago by Mjumbe Poe

Cc: Mjumbe Poe added

comment:9 Changed 3 years ago by Victor Hooi

Cc: victorhooi@… added

Hi,
Curious - what is the status on this ticket?
Are there still blockers on it being accepted?
Cheers,
Victor

comment:10 Changed 3 years ago by Dominic Rodger

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 Changed 3 years ago by Dominic Rodger

Cc: internet@… added

comment:12 Changed 3 years ago by flisky

Cc: flisky added

comment:13 Changed 2 years ago by maestrofjp

Cc: maestrofjp added

comment:14 Changed 2 years ago by Tim Graham

This should be easier after removing some of the logic from the form in #13147.

comment:15 Changed 2 years ago by Loic Bistuer <loic.bistuer@…>

In 671e0c937c112b1908048b4d8deb14c0116b8946:

Further fix the release notes for refs #13147.

Mention of custom user models has been removed since UserCreationForm
didn't support custom user models anyway. Refs #19353.

comment:16 Changed 16 months ago by Berker Peksag

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.

Last edited 16 months ago by Berker Peksag (previous) (diff)

comment:17 Changed 16 months ago by Berker Peksag

Cc: berker.peksag@… added

comment:18 Changed 16 months ago by Tim Graham

Sure, might want to also consider including some documentation updates from bmispelon's branch in comment 5.

comment:19 Changed 16 months ago by Baptiste Mispelon

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 Changed 10 months ago by Berker Peksag

Needs documentation: unset
Owner: changed from nobody to Berker Peksag
Status: newassigned

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:21 Changed 10 months ago by Tim Graham <timograham@…>

In f0425c7:

Refs #19353 -- Added tests for using custom user models with built-in auth forms.

Also updated topics/auth/customizing.txt to reflect that subclasses of
UserCreationForm and UserChangeForm can be used with custom user models.

Thanks Baptiste Mispelon for the initial documentation.

comment:22 Changed 10 months ago by Tim Graham <timograham@…>

In f78892f2:

[1.9.x] Refs #19353 -- Added tests for using custom user models with built-in auth forms.

Also updated topics/auth/customizing.txt to reflect that subclasses of
UserCreationForm and UserChangeForm can be used with custom user models.

Thanks Baptiste Mispelon for the initial documentation.

Backport of f0425c72601f466c6a71518749c6d15b94945514 from master

comment:23 Changed 10 months ago by Tim Graham

Resolution: fixed
Status: assignedclosed

If there are any further improvements, let's open a new ticket. Thanks!

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