Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #7 required email constraint #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

trottomv
Copy link
Contributor

@trottomv trottomv commented Nov 21, 2023

fix #7

@trottomv
Copy link
Contributor Author

trottomv commented Nov 23, 2023

Hi @carltongibson
I opened this pull request with a bit of calm.
I admit it's quite extensive, and perhaps some parts might be out of scope. Many things could potentially fit better in the README than in the app's code. I don't expect a merge, but I'm leaving it for you as a reference in case it sparks any ideas. Feel free to trash it or do whatever you prefer; I won't be offended :)

I used django-unique-user-email solution in a django project without install the app, and this is roughly what I added to make it functional for my use case.

Some personal considerations:

  • The CheckConstraint on User.Meta might be redundant, and it might be sufficient to make the email field required in the user creation/modification form. However, personally, I feel more confident and prefer having a lower-level constraint rather than in the form, or even both in combination.
  • The entire custom UserAdmin section might fit well in the README rather than in the code.

Furthermore, after some checks and experimentation done together with @pauloxnet, we realized that installing the app or introducing this solution into an existing project that's already running (with auth migrations already applied and the auth_user table data already populated) could have some unintended effects.
It would be a nice challenge to find a solution to support this specific case.

@carltongibson
Copy link
Owner

Hey @trottomv — Thanks 👍 — I've seen it. I just haven't got to it. 😅

Feel free to trash it or do whatever you prefer; I won't be offended :)

Deal 😉

The CheckConstraint on User.Meta might be redundant...

Yep, in normal usage the form is enough. But belt-and-braces: we want the constraint too. Right?
(Insert Anakin Skywalker meme)

... could have some unintended effects ...

Like what? Sure you have to migrate the table (add the constraint) but (on the assumption your data isn't incompatible) what's to stop you migrating forward and backward multiple times (as needed) — you add the constraint, you remove it?
(Not saying there isn't any unintended effects, just asking 😜)

@trottomv
Copy link
Contributor Author

trottomv commented Nov 23, 2023

Like what? Sure you have to migrate the table (add the constraint) but (on the assumption your data isn't incompatible) what's to stop you migrating forward and backward multiple times (as needed) — you add the constraint, you remove it? (Not saying there isn't any unintended effects, just asking 😜)

Scenario:
The auth migrations are already applied, and the auth_user table already populated with this data.

ID username email
1 testuser1 [email protected]
2 testuser2 ""
3 testuser3 ""

Applying the django-unique-user-email migrations would raise an IntegrityError on users existing data. Because the UniqueConstraint on the User.email field setted on unique_user_email.apps would prevent the application of migration 0001.
In summary, two users with an empty email field are sufficient to break migration 0001.

@carltongibson
Copy link
Owner

Oh, yeah, sure. If you've got non-unique data then you're going to hit an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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