Make email field in user form required #449
Conversation
There was a problem hiding this comment.
Thanks for trying this!
Apart from a few reasons why the code cannot work like this (see my comments below), we have the more general problem that migrating the production user database to a custom model in mid-project is very complicated. I mean you probably read the documentation about abstract base users, which include this paragraph:
Changing AUTH_USER_MODEL after you’ve created database tables is significantly more difficult since it affects foreign keys and many-to-many relationships, for example.
This change can’t be done automatically and requires manually fixing your schema, moving your data from the old user table, and possibly manually reapplying some migrations. See #25313 for an outline of the steps.
Which raises the question whether #401 is important enough to justify the effort of this approach in this case. In my opinion, it isn't, so I'd look for other ways (which are maybe less "clean" but also way less time consuming).
The main concern for this is the user form in the cms and not the createsuperuser management command. So I think it would also be fine to just enforce this change in the admin by customizing only the form and leave the model unchanged (so no database-level enforcement of this rule) or to monkeypatch the model (similar to how we did for the group model).
Is extending the AbstractUser model the right way to change the "create new user" form? That was the result of my research, but I'm not sure that's how it works
Is is one option, but not the only one (and not the easiest one, as stated above). Also, introducing a model does not automatically change the "create new user" form. Django is able to derive form from models, but they're not the same thing.
I was wondering if we still need username as the primary identification. I would argue we could also take emails as unique identification or do a hybrid solution where either username or email can be taken as information. Although I realize that it's probably to late to change it to email as sole identification.
Was I correct by trying to add an email field as part of the "create new user" form?
Once #401 is resolved, we could also think about using emails for authentication, yes. But this does not replace the username as primary identification, it's just an additional authentication backend. E.g. have a look how we did it for integreat-cms.
Was I correct by trying to add an email field as part of the "create new user" form?
Not sure what you mean by this, you did not change the "create new user" form whatsoever. You changed the create_user method of the UserManager, but the manager has nothing to do with the form.
| @@ -12,4 +12,5 @@ | |||
| "GroupAPIKeyAdmin", | |||
| "FeedbackAdmin", | |||
| "SponsorAdmin", | |||
| "UserAdmin", | |||
There was a problem hiding this comment.
Changing __all__ without importing the UserAdmin here is completely useless. You have to insert from .user_admin import UserAdmin in this file if you want to enable other modules to do a from lunes_cms.cms.admins import UserAdmin.
| @@ -0,0 +1,7 @@ | |||
| from django.contrib import admin | |||
|
|
|||
| from ..models import RegisterUserForm | |||
There was a problem hiding this comment.
What does this line do? 1. There is no RegisterUserForm in our code base and 2. forms are forms and not models.
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
Just adding the autogenerated migration won't magically migrate all existing users to the new model.
| from ..models import RegisterUserForm | ||
|
|
||
|
|
||
| class UserAdmin(admin.ModelAdmin): |
There was a problem hiding this comment.
New admins also have to be registered in lunes_cms/cms/admin.py, otherwise they are just a few lines of code that are never executed.
| class UserManager(BaseUserManager): | ||
| def create_user(self, username, email, password): | ||
| if not email: | ||
| raise ValueError("_('Users must have an email address')") | ||
| if not password: | ||
| raise ValueError("_('Users must have a password')") | ||
| user = self.model(email=self.normalize_email(email)) | ||
| user.set_username(username) | ||
| user.set_password(password) | ||
| user.save() | ||
| return user |
There was a problem hiding this comment.
No sure if we need a custom manager for this. The main concern is the UI in the CMS, not the createsuperuser management command.
| username = models.CharField(default="", unique=True, max_length=30) | ||
| email = models.EmailField(default="") |
There was a problem hiding this comment.
A default value of "" sounds pretty unreasonable to me.
| email = models.EmailField(default="") | ||
|
|
||
| USERNAME_FIELD = "username" | ||
| REQUIRED_FIELDS = ["email", "password"] |
There was a problem hiding this comment.
REQUIRED_FIELDSmust contain all required fields on your user model, but should not contain theUSERNAME_FIELDorpasswordas these fields will always be prompted for.
Short description
Once this PR is finished it's supposed to make the email field in the user form required. Therefore I would suggest to also add an email field to the "create new user"-form.
Proposed changes
I noticed that my changes didn't work as I expected them to. Now I feel a bit stuck and have a few questions:
Thank you in advance for bringing clearance to my confusion 😄
Resolved issues
Fixes: #401