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.

Sign up for GitHub

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

Jump to bottom

Rework email confirmations #44

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from
Draft

Rework email confirmations #44

wants to merge 20 commits into from

Conversation

merwok
Copy link
Contributor

@merwok merwok commented Oct 18, 2018 •
edited
Loading

Fixes #38

@merwok merwok self-assigned this Oct 24, 2018
@merwok merwok force-pushed the feature/rework-email-tokens branch 2 times, most recently from 4eced47 to 888578d Compare October 30, 2018 16:52
@merwok merwok force-pushed the feature/rework-email-tokens branch from cf07157 to 4eced47 Compare November 6, 2018 16:32
@merwok merwok force-pushed the feature/rework-email-tokens branch from 9331606 to 526fd6a Compare November 7, 2018 01:35
merwok
merwok commented Nov 11, 2018
View reviewed changes
rest_auth_toolkit/views.py
user = deserializer.save()

if self.email_confirmation_class is None:
raise MissingSetting('email_confirmation_class')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check could be moved to SignupView.as_view, so that the problem is visible when the site starts rather than during the first request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better: #52

just1602
just1602 approved these changes Nov 11, 2018
View reviewed changes
Copy link

@just1602 just1602 left a comment

Choose a reason for hiding this comment

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

Clean! Je crois que je ferais le changement pour que la validation se fasse dans la méthode as_view() immédiatement, mais sinon c'est cool !

@merwok
Copy link
Contributor Author

merwok commented Nov 12, 2018

Agreed on doing this in the same PR!

merwok
merwok commented Nov 19, 2018
View reviewed changes
rest_auth_toolkit/serializers.py
except EmailConfirmation.DoesNotExist:
msg = _('Invalid link')
except EmailConfirmation.IsExpired:
# FIXME it's not possible to register with the same email
Copy link
Contributor Author

Choose a reason for hiding this comment

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

address this

Copy link

Choose a reason for hiding this comment

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

We should check for expired confirmation, but it shouldn't be an error related to a duplicate email, tho? A user shouldn't be able to create an account with an email related to another email, tho ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that’s the problem with saving the User/Account before confirmation :(

After #19 and #30 this will be cleaner, but here I’m looking for something simpler but correct for the 0.10 version.

@merwok
Copy link
Contributor Author

merwok commented Nov 23, 2018

Ok so the plan is:

@merwok
Copy link
Contributor Author

merwok commented Aug 13, 2019

This has been open too long. I think the new endpoint should be another ticket.

@merwok merwok mentioned this pull request Aug 13, 2019
Draft
@merwok merwok requested review from gkijko and removed request for mathieuhentges and JulienLabonte August 13, 2019 18:43
@merwok merwok changed the title WIP rework email confirmations Rework email confirmations Aug 13, 2019
@merwok merwok changed the base branch from develop to master December 3, 2020 03:00
@merwok merwok changed the base branch from master to main December 8, 2020 23:31
@merwok merwok marked this pull request as draft January 13, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@just1602 just1602 just1602 approved these changes

@gkijko gkijko gkijko approved these changes

Assignees

@merwok merwok

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rework email confirmation
3 participants
@merwok @just1602 @gkijko