incuna / django-user-management

User management model mixins and api views.
BSD 2-Clause "Simplified" License
57 stars 24 forks source link

Require email field to not be blank in EmailUserMixin #165

Closed henrahmagix closed 7 years ago

henrahmagix commented 7 years ago

https://github.com/incuna/django-user-management/blob/c5cccfa3bc815d1d5923d2a4f96c8ab218f46925/user_management/models/mixins.py#L59-L63

Just encountered an odd thing where I could create a user with no parameters, and there was no error. It doesn't feel right that a blank email isn't validated as false.

@incuna/backend @meshy What do you think?

from django.contrib.auth.models import AbstractBaseUser, PermissionsMixin
from user_management.models.mixins import VerifyEmailMixin

class User(VerifyEmailMixin, PermissionsMixin, AbstractBaseUser):
    pass
LilyFoote commented 7 years ago

How were you creating the user/validating it?

henrahmagix commented 7 years ago

Through python manage.py shell_plus:

User.objects.create()

@KevinEtienne was helping me add a user to a very new system (so I could dumpdata into a fixture for some e2e tests) and said we could call .create() with no parameters to see what errors come up, then we'll know what's required. He then remarked that it was odd there were no errors, that in the least email should not accept blank.

If this is better suited as another mixin instead of changing EmailUserMixin, or there was a decision to allow blank, then 👍 so I'm just checking if it wasn't a mistake =)

adam-thomas commented 7 years ago

Here's the email field.. It should be defaulting to blank=False, but it might be that calling User.objects.create() pre-fills it with an empty string (since it's a CharField).

@henrahmagix I imagine that if you run the same command again you'll get an error about how email='' already exists?

henrahmagix commented 7 years ago

It should be defaulting to blank=False, but it might be that calling User.objects.create() pre-fills it with an empty string (since it's a CharField).

If that's the case, then there's no issue? I'm good with that =)

I imagine that if you run the same command again you'll get an error about how email='' already exists?

Yarp.

adam-thomas commented 7 years ago

There's sort of technically an issue. In practice, our form fields won't let you submit an empty email address, and a 400 is much friendlier than a 500 (which is what you get from doing it on the model), so it's not a big deal.

LilyFoote commented 7 years ago

I think it's because model validation isn't run by default, except in a ModelForm. https://docs.djangoproject.com/en/1.10/ref/models/instances/#validating-objects

adam-thomas commented 7 years ago

^ What he said.

adam-thomas commented 7 years ago

(trying to find the right Hot Fuzz gif and failing)

adam-thomas commented 7 years ago

Anyway, s' all good :)

henrahmagix commented 7 years ago

Thanks guys!