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: allow users update their account email address with proper validation. #4520

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

apsinghdev
Copy link
Contributor

@apsinghdev apsinghdev commented Nov 22, 2024

fixes: #4237

This PR implements the feature to allow users to update their email. (Draft)

Screen.Recording.2024-12-01.at.10.25.19.PM.mov

Copy link

vercel bot commented Nov 22, 2024

@apsinghdev is attempting to deploy a commit to the polar-sh Team on Vercel.

A member of the Team first needs to authorize it.

@apsinghdev apsinghdev changed the title Feat/allow email update feat: allow users update their account email address with proper validation. Nov 22, 2024
@apsinghdev
Copy link
Contributor Author

apsinghdev commented Nov 23, 2024

@frankie567 I've raised this draft PR that contains backend code. please have a look and let me know if it make sense.

(There are further improvements like tags that I'll make this week)

Copy link
Member

@frankie567 frankie567 left a comment

Choose a reason for hiding this comment

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

That's great work @apsinghdev 👍 Let's change a few things to make it really tailored for email verification :)

Be sure also to run linters:

uv run task lint && uv run task lint_types

server/polar/models/email_verification.py Outdated Show resolved Hide resolved
server/polar/email_update/service.py Outdated Show resolved Hide resolved
server/polar/email_update/service.py Outdated Show resolved Hide resolved
server/polar/email_update/service.py Outdated Show resolved Hide resolved
server/polar/email_update/service.py Outdated Show resolved Hide resolved
server/polar/email_update/endpoints.py Outdated Show resolved Hide resolved
server/polar/email_update/endpoints.py Outdated Show resolved Hide resolved
server/polar/email_update/endpoints.py Outdated Show resolved Hide resolved
@apsinghdev
Copy link
Contributor Author

That's great work @apsinghdev 👍 Let's change a few things to make it really tailored for email verification :)

Be sure also to run linters:

uv run task lint && uv run task lint_types

@frankie567 Thanks for the reviews. I'll fix these very soon.

@apsinghdev apsinghdev force-pushed the feat/allow-email-update branch from 6c85b89 to 8a54988 Compare December 1, 2024 17:44
@apsinghdev
Copy link
Contributor Author

apsinghdev commented Dec 1, 2024

@frankie567 I've made the updates to apply reviews and implemented client-side logic and UI. Please have a look whenever you are free and let me know your thoughts.

Demo:

Screen.Recording.2024-12-01.at.10.25.19.PM.mov

Copy link
Member

@frankie567 frankie567 left a comment

Choose a reason for hiding this comment

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

We're getting there, great work @apsinghdev 👍 Just a few adjustments and we should be able to land this.

server/polar/email_update/schemas.py Outdated Show resolved Hide resolved
server/polar/email_update/service.py Outdated Show resolved Hide resolved
server/polar/email_update/service.py Outdated Show resolved Hide resolved
@apsinghdev apsinghdev force-pushed the feat/allow-email-update branch from 7c77a76 to b001aff Compare December 8, 2024 17:35
Copy link

vercel bot commented Dec 8, 2024

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

Name Status Preview Comments Updated (UTC)
polar ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 8:32am
polar-sandbox 🛑 Canceled (Inspect) Dec 18, 2024 8:32am

@apsinghdev
Copy link
Contributor Author

@frankie567 I have made the changes. Please have a look whenever you get the chance.

Demo:

Screen.Recording.2024-12-08.at.10.40.52.PM.1.mov

@apsinghdev
Copy link
Contributor Author

(Already posted on discord, writing here just for the reference)

You might have a question about why there is a verify-email route. Actually, I tried to keep the whole update process in the settings itself but it was causing a problem of not updating the email in the first render because of the async nature of react hooks. I had to refresh the page to verify the email.

I tried these ways to solve it:

Catching the token using urlSeachParams and then triggering a hit to API to verify

Creating a route.ts to intercept the request midway and hit api

Using server actions.

In all of these methods, We had to depend on client-side rendering to provide us the token in the URL to take further actions to verify but by default, the very first render doesn't set the token properly. So I considered creating a server-side page verify-email to avoid dependency on client-side rendering and verify the user.

If you have a better idea to implement it, please let me know. I'll be happy to make changes.

@frankie567 frankie567 force-pushed the feat/allow-email-update branch from f59a2d0 to c19fe2a Compare December 16, 2024 10:55
@frankie567
Copy link
Member

Hey @apsinghdev 👋

That's great work, thanks! So, I took the liberty to rebase your PR and make some slight changes to the UI. Basically, it's now nested under the Signin connections, but the logic remains the same (really liked your approach with a Record of components to render depending on the status ;) )

Capture d’écran 2024-12-16 à 11 48 25 Capture d’écran 2024-12-16 à 11 48 35 Capture d’écran 2024-12-16 à 11 48 38 Capture d’écran 2024-12-16 à 11 49 25

During my tests, I found one big issue we need to resolve: when requesting the email change, we do not check if the email already exists in the database, which causes an issue when trying to update the email.

Could you do that? Basically, at the beginning of request_email_update, check if an email already exists in DB, and raise a PolarRequestValidationError if that's the case. Then, display that error message in the UI.

Thank you 🙏

@apsinghdev
Copy link
Contributor Author

@frankie567 Thank you for the reviews. I'll make the changes very soon. 🙂

@apsinghdev apsinghdev force-pushed the feat/allow-email-update branch from c19fe2a to 492125b Compare December 17, 2024 17:44
@apsinghdev
Copy link
Contributor Author

apsinghdev commented Dec 17, 2024

Hi @frankie567, I've made the necessary changes to check if an email already exists or not. Here is how it looks:

Screen.Recording.2024-12-17.at.11.14.32.PM.mov

Please take a look when you are free.

@frankie567 frankie567 force-pushed the feat/allow-email-update branch from bcd735d to 412dbbc Compare December 18, 2024 08:18
@frankie567 frankie567 enabled auto-merge (rebase) December 18, 2024 08:22
@frankie567
Copy link
Member

Thank you @apsinghdev 👍 Made a few tweaks + a rebase, it's good to go now 😄

Thank you for your hard work 🙇

@frankie567 frankie567 disabled auto-merge December 18, 2024 08:31
@frankie567 frankie567 merged commit a1f69fd into polarsource:main Dec 18, 2024
6 of 8 checks passed
@vercel vercel bot temporarily deployed to Preview – polar-sandbox December 18, 2024 08:32 Inactive
@apsinghdev
Copy link
Contributor Author

Thank you @apsinghdev 👍 Made a few tweaks + a rebase, it's good to go now 😄

Thank you for your hard work 🙇

Thank you so much for your valuable feedbacks. 🙏😊

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.

Allow to update account email address
2 participants