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

feat: email verification #925

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat: email verification #925

wants to merge 8 commits into from

Conversation

moshfeu
Copy link
Member

@moshfeu moshfeu commented Apr 28, 2022

The flow

  • User register
  • A verification email sent by auth0
  • If the user still not verified, we show them a modal

Screen Shot 2022-05-04 at 12 50 18

- Clicking the button will send the user another verification email

Screen Shot 2022-05-04 at 12 51 48

Product decisions

  • User who is not verified can only do what anonymous user can do.
    • The only UI impact is that the login / register link is replaced by an empty avatar so the user can logout
  • All the users who are not verified, including existing, must verify their email (we expect to more support requests)

Server PR - Coding-Coach/find-a-mentor-api#249

todo: resend email logic
@vercel
Copy link

vercel bot commented Apr 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
find-a-mentor ❌ Failed (Inspect) May 6, 2022 at 6:13AM (UTC)

todo: resend verification email
todo: handle verification callback - success / error

type UserProviderContext = {
isAdmin: boolean;
isMentor: boolean;
isLoading: boolean;
currentUser?: User;
emailVerifedInfo?: EmailNotVerifiedInfo;

Choose a reason for hiding this comment

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

Typo: emailVerifiedInfo

Copy link
Member Author

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 ;)

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!

Copy link
Member Author

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.

Choose a reason for hiding this comment

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

Looks good 🧙

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +118 to +122
ApiService.clearCurrentUserFromStorage();
}

// because we need to call it from authContext which doesn't have access to ApiService
static clearCurrentUserFromStorage = () => {
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. Thanks.

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