-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
feat: email verification #925
base: master
Are you sure you want to change the base?
Conversation
todo: resend email logic
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
todo: resend verification email
todo: handle verification callback - success / error
|
||
type UserProviderContext = { | ||
isAdmin: boolean; | ||
isMentor: boolean; | ||
isLoading: boolean; | ||
currentUser?: User; | ||
emailVerifedInfo?: EmailNotVerifiedInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: emailVerifiedInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
If this is the only issue with the changes I made in the auth0 area, I'm happy ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes made regarding Auth0 are limited I would say. But yeah, looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I mostly interested in a feedback about the verification email process.
If you can take a look on the server PR too it would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🧙
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
ApiService.clearCurrentUserFromStorage(); | ||
} | ||
|
||
// because we need to call it from authContext which doesn't have access to ApiService | ||
static clearCurrentUserFromStorage = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this static method still necessary after adding forgetUser
to the auth util?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. forgetUser
calls this function.
The problem is that auth/utils not always has a reference to API so if it has reference it calls to clearCurrentUser
but if not (it means that apiService is not initiated), it calls clearCurrentUserFromStorage
directly.
We definitely should improve the design / structure of the auth / user area. We 2 providers and 2 services.
I did some research about auth0 react wrapper, it can help us clear our code but I didn't want to include this refactor in this PR to avoid big changes at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Thanks.
The flow
- Clicking the button will send the user another verification email
Product decisions
Server PR - Coding-Coach/find-a-mentor-api#249