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 upon Signup #288

Merged
merged 26 commits into from
Aug 13, 2024
Merged

Conversation

solderq35
Copy link
Contributor

When joining new team

@solderq35
Copy link
Contributor Author

solderq35 commented Jul 14, 2024

Demo Vid: https://youtube.com/shorts/e6q_DAK7Q_g

Timeout didn't quite work so just clicked "join team" twice I think

@solderq35
Copy link
Contributor Author

solderq35 commented Jul 14, 2024

TODO:

  • Find a better way to update currentUser.emailVerified
  • Use Alert.js file for alert or dialogs, don't use console.error

@solderq35
Copy link
Contributor Author

I think the current changes in Auth.js are extraneous and can be removed

@FrankreedX
Copy link
Contributor

Find a better way to update currentUser.emailVerified

Maybe onAuthStateChanged?

@solderq35
Copy link
Contributor Author

New Demo Vid: https://youtu.be/yzMtmMPuigU

Intended Workflow: New User signs up (email verification sent) > User taps "Join Team" button (or taps "Resend Verification Email" if needed) > User is logged into the app and redirected to homescreen (onIdTokenChanged asynchronously watches for when the email is verified on the user server side)

@solderq35 solderq35 marked this pull request as ready for review July 23, 2024 06:29
@solderq35
Copy link
Contributor Author

PS does the "join team" and "resend email verification" buttons need debouncing? I noticed that "join team" seemed to trigger multiple times sometimes (like 3 times in a row rapidly, hence janky navigation after tapping "join team" button).

@FrankreedX
Copy link
Contributor

PS does the "join team" and "resend email verification" buttons need debouncing? I noticed that "join team" seemed to trigger multiple times sometimes (like 3 times in a row rapidly, hence janky navigation after tapping "join team" button).

Yes, and it will be addressed in either debounce 2 (if simple) or debounce-n (if complex) once this PR is merged

Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

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

Works. I encountered an issue where verification success is printed twice.
Screenshot 2024-07-23 at 2 44 38 PM

app/(auth)/signup.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
@FrankreedX
Copy link
Contributor

What is your plan to add button spinners for all of this PR's operations?

@solderq35
Copy link
Contributor Author

What is your plan to add button spinners for all of this PR's operations?

Assuming that "Join Team" button is hidden to user until email is verified, I guess we could make the "Resend Verification Email" button show a spinner animation until the email is finished sending?

For verified users, I suppose the "Join Team" button could show a spinner animation until all async operations were awaited (firestore operations etc)

@FrankreedX
Copy link
Contributor

Ye it took a very long time for me when I tested it. It's helpful to know that the app received my input and is doing work instead of just trusting it.

@solderq35
Copy link
Contributor Author

solderq35 commented Jul 26, 2024

07-25-2024 Progress

  • Pulldown refresh
  • Hide "join team" button until user is verified?
  • spinner animation ("Loading") while Verification email is being sent
    • 08-10-2024 Edit: The last 2 tasks should be done as of today

Screenshot

image

@solderq35
Copy link
Contributor Author

Fixed the below signout error (see screenshot), although not sure if this will cause excess re renders etc, maybe there is better solution.

image

@FrankreedX FrankreedX changed the base branch from main to Gehrkej/fix/add-email-to-blacklist August 5, 2024 02:24
Base automatically changed from Gehrkej/fix/add-email-to-blacklist to main August 5, 2024 17:24
@solderq35
Copy link
Contributor Author

solderq35 commented Aug 7, 2024

image

Kind of got loading spinner working for sending verification email, and also kind of fixed bug where it was inconsistent if the new user would be directed to chooseTeam screen upon signup.

TODO:

  • Cleanup code / log statements
  • Maybe switch everything to auth.currentUser.emailVerified as it seems to work more reliably (given persistent login credentials)

@FrankreedX
Copy link
Contributor

you gotta set the "textColor" for the spinner to change

@solderq35
Copy link
Contributor Author

Changed spinner color to white:
image

I removed the dependency array from the unregisterAuthObserver useEffect due to it bugging out.

Basically you need to call user.reload() in order for the updated status to register. If you also put auth.currentUser (or whatever) in the useEffect dependency array, it causes an infinite feedback loop.

So now it does work but the user has to manually pulldown to refresh after they verified email

Maybe helpful for the useEffect / emailVerified / onIdTokenChanged issue:

@solderq35
Copy link
Contributor Author

I think the useEffect works as intended now (it automatically detects when you verified email and then shows "Join Team" button afterwards).

To resolve the issues mentioned in my previous comment, I had to set up a setInterval to reload the user and check for updates every 10 seconds.

Demo Video (read Youtube video description): https://youtu.be/KpHN71xhFfE

@solderq35 solderq35 requested a review from FrankreedX August 9, 2024 03:51
@solderq35
Copy link
Contributor Author

PS: I tried putting showSnackBar in the useEffect dependency Array, but it caused the "Email successfully verified" popup to not go away for some reason later, especially if I had waited about 20 or 30 seconds after signup to verify email.

So I am leaving it out of the dependency array for now.

image

Gehrkej
Gehrkej previously requested changes Aug 9, 2024
Copy link
Contributor

@Gehrkej Gehrkej left a comment

Choose a reason for hiding this comment

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

Overall, the functionality works well as I am able to verify my email and sign in. We will want to figure out the Auto send functionality before merging. Additionally, for DX, we will probably want an easy way to disable this for testing purposes as merging this in will limit the amount of accounts we will have access to.

app/(auth)/signup.js Show resolved Hide resolved
app/(auth)/signup.js Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Show resolved Hide resolved
dbOperations/hooks/useUserInfo.js Outdated Show resolved Hide resolved
Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

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

Good job on this! Especially the refresh in the background stuff. It feels really responsive! I only have minor complaints, and certainly have my work cut out to merge this with my stack of PRs.

dbOperations/hooks/useUserInfo.js Outdated Show resolved Hide resolved
app/segments/(team)/chooseTeam.js Outdated Show resolved Hide resolved
dbOperations/hooks/useUserInfo.js Outdated Show resolved Hide resolved
@solderq35
Copy link
Contributor Author

Had to change the context/Auth.js useEffect from onAuthStateChanged to onIdTokenChanged in order to get currentUserVerified / setCurrentUserVerified to work as intended. Hopefully that doesn't cause too many re renders etc.

Copy link
Contributor

@FrankreedX FrankreedX left a comment

Choose a reason for hiding this comment

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

lgtm

@solderq35 solderq35 dismissed Gehrkej’s stale review August 13, 2024 00:19

Changes are resolved (I think)

@solderq35 solderq35 merged commit d009362 into main Aug 13, 2024
1 check passed
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