User.email field is not mandatory and remain blank=True

[trottomv]

Hi @carltongibson,
I'm here again, because your solution seems really interesting to me. I noticed, though, that the email field remains with blank=True parameter and since it's not a required field it's not subject to any validation.
I've already drafted some code on my forked repo. There might still be somethings to fix, such as the split of the migrations, but I would like to have your feedback on it before opening a pull request on this repo.

If you want, you can take a look at this link in the meantime.
https://github.com/trottomv/django-unique-user-email/pull/1/files

The problem: without a constraint on empty email, adding two user with an empty email to database is raised an IntegrityError from the existent UniqueConstraint, so the field should have another constraint to check the email value is not empty.

first solution on my mind: ok, easy... add some blank=False handler on User.email from unique_user_email.apps configurator
but, no, is not so easy because the MigrationAutoDetector remove it (and something similar to the problem reported on issue #5 happens)

workaround solution: Add also a CheckConstraint as you can see into draft code linked before

CheckConstraint(
    check=~Q(email__exact=""),
    name="check_required_email",
    violation_error_message="A valid 'Email address' is required.",     
),

[carltongibson]

Hey @trottomv. Yes. Of course. Since email is not null=True this will come up.

I think the additional constraint is required, and should be mentioned in the README briefly too.

Good spot! 😜

[carltongibson]

Tests for the ModelForm behaviour should check this too at that level.

We can mung the validation exclusions at the form level, so that may need some thought.

[trottomv]

Tests for the ModelForm behaviour should check this too at that level.

We can mung the validation exclusions at the form level, so that may need some thought.

The question of the forms could open a nice chapter of reflection, which perhaps would be better managed separately (but I'm not quite sure).
But, perhaps, acting further on the unique_user_email.apps configurator by setting

User.USERNAME_FIELD = "email"
User.REQUIRED_FIELDS = []

could be sufficient to guarantee login with the email and could avoid and make unnecessary the whole customized part on forms.py and backend.py, unless that the original intent to grant login with "username or email" is a mandatory requirement.

[carltongibson]

I think that's going off the right path.

The idea is simply to allow login by email with the default user model. Once we start messing too much we've lost the benefit of that.

In general here, overriding _get_validation_exclusions() on the form to require the field you want validated is usually sufficient.

Happy to input if you're unsure... — If you put together the test case we can see what's needed to make it pass 😜

[carltongibson]

Catching this issue is a nice to have: nobody who wants login by email is going to let users sign up without an email address 😜

But, yes, let's catch it.

[carltongibson]

Hi @trottomv — I merged #6 and pushed a new release. Thanks for the input there again! 🎁

I didn't want to rush this one in: it's a quirk, but not really a show-stopper, and I'd like to do one thing once here 👍

[trottomv]

Hi @carltongibson
I took a look at the _get_validation_exclusions() method. I admit I'm not very experienced with forms, so feel free to correct me if you think I'm not heading in the right direction. However, it seems to me that introducing this override may not be necessary, and I'd like to share a few considerations:

In Django's BaseModelFormSet, the _get_validation_exclusions() method is called at line 811
https://github.com/django/django/blob/a2769a68ea27242dc70ec7734c4ed38932fe46da/django/forms/models.py#L811

Just below at line 814, the exclude is passed to form.instance._get_unique_checks() but with the parameter include_meta_constraints=True. This should be sufficient to handle the validations of the constraints setted at a lower level on both the User model and the database.

You don't think so? 🤔

[carltongibson]

Not sure without a test case 😜

That's where I'd begin. Let's have a test that fails on main entering a blank email. (Form should fail is_valid(), like the existing test.)

Then if it passes when we add the constraint that'd be job done 😜

[trottomv]

You're right, the draft code linked before is not ready for a PR yet, and there are still some things to be fixed, including missing tests. However, from a human check on Django admin, it seems that the user creation/edit form is behaving correctly.

[carltongibson]

OK, great.

Sounds like this is the right path. If you want to open a PR when you're happy I will take a look.

Thanks @trottomv