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

Fixed Incompatibility with django-allauth v0.55.0 #536

Closed
wants to merge 5 commits into from

Conversation

brandon-kong
Copy link

dj-rest-auth is not compatible with django-allauth's latest version. This PR fixes that issue upon the newest release of django-allauth v0.55.0.

I created a email_address_exists method in dj_rest_auth's utils with the same implementation.

Copy link
Author

@brandon-kong brandon-kong Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the maintainer finalizes this PR, change (in demo/requirements.pip)

dj-rest-auth @ git+https://github.com/brandon-kong/dj-rest-auth.git@master

to

dj-rest-auth @ git+https://github.com/iMerica/dj-rest-auth.git@master

I tweaked it for testing purposes, but after all changes are verified, it should be linking to this repo.

@flange-ipb
Copy link
Contributor

Hi @brandon-kong,
for the sake of maintainability, wouldn't it be better to adopt the drop-in replacement from django-allauth? If you look at commit 9fccdd0 email_address_exists is replaced by EmailAddress.objects.is_verified.

@brandon-kong
Copy link
Author

Hi @brandon-kong, for the sake of maintainability, wouldn't it be better to adopt the drop-in replacement from django-allauth? If you look at commit 9fccdd0 email_address_exists is replaced by EmailAddress.objects.is_verified.

Good catch, I completely missed their new replacement.

@brandon-kong
Copy link
Author

brandon-kong commented Aug 23, 2023

Hi @brandon-kong, for the sake of maintainability, wouldn't it be better to adopt the drop-in replacement from django-allauth? If you look at commit 9fccdd0 email_address_exists is replaced by EmailAddress.objects.is_verified.

Actually, is that the intended use-case? As I'm testing it now, users would be able to create accounts with already-existing emails if the email is not verified; I assume that isn't the intended result. Wouldn't it make more sense to filter for emails existing rather than filtering for verified existing emails? I'll revert my code back to the previous commit I pushed

@Dresdn
Copy link
Contributor

Dresdn commented Aug 31, 2023

I had started to work on this, and then after seeing your direction, I decided just to commit and opened #539.

Actually, is that the intended use-case? As I'm testing it now, users would be able to create accounts with already-existing emails if the email is not verified; I assume that isn't the intended result. Wouldn't it make more sense to filter for emails existing rather than filtering for verified existing emails? I'll revert my code back to the previous commit I pushed

I disagree and think that is the intended result. With the method you outlined, I could mass register all emails and effectively block users from signing up. Instead, you should allow duplicate email registrations, as only one will actually get verified. Then, you can run a cleanup job to remove all verified=False objects after a certain amount of time.

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.

3 participants