-
Notifications
You must be signed in to change notification settings - Fork 330
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
User Registration with Profile Fields #1776
User Registration with Profile Fields #1776
Conversation
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 to me! Reviewed everything up to 5d91432 in 1 minute and 35 seconds
More details
- Looked at
163
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. py/core/database/users.py:291
- Draft comment:
Ensure that the fields 'name', 'bio', and 'profile_picture' are handled correctly when they are None. This can be done by using a default value or handling null values from the database. - Reason this comment was not posted:
Comment did not seem useful.
2. py/core/main/services/auth_service.py:46
- Draft comment:
Consider adding validation for the 'name', 'bio', and 'profile_picture' parameters to ensure they meet any necessary requirements (e.g., length, format). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The validation suggestion seems reasonable at first glance, but several things make me doubt its usefulness: 1) These parameters are Optional, so they can be None 2) The same parameters are used elsewhere without validation 3) The validation would likely be handled in the auth provider layer, not here 4) Without knowing the requirements, we can't be sure validation is even needed 5) The comment is speculative - it's not pointing out a clear issue.
I might be wrong about where validation should occur - maybe this service layer is exactly where it belongs. Also, lack of validation elsewhere doesn't mean it's not needed here.
While those are fair points, the comment still fails our core rules: it's speculative ("if needed"), not clearly actionable (no specific requirements given), and assumes validation isn't handled elsewhere.
The comment should be deleted because it's speculative and not clearly actionable without more context about validation requirements and where validation should occur in the architecture.
3. py/core/providers/auth/r2r_auth.py:198
- Draft comment:
Consider adding validation for the 'name', 'bio', and 'profile_picture' parameters to ensure they meet any necessary requirements (e.g., length, format). - Reason this comment was not posted:
Marked as duplicate.
4. py/core/providers/auth/supabase.py:75
- Draft comment:
Consider adding validation for the 'name', 'bio', and 'profile_picture' parameters to ensure they meet any necessary requirements (e.g., length, format). - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_dqxhC8Goll1C3AXO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and 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.
Looks pretty good! A few small changes requested. Can you also make sure to run poetry run pre-commit run --all-files
from the /py directory? No need to worry about the mypy type checking errors, as these are on main, but the rest should all pass.
py/core/main/api/v3/users_router.py
Outdated
@@ -129,15 +129,16 @@ def validate_password(password: str) -> bool: | |||
validate_password(password) | |||
|
|||
registration_response = await self.services.auth.register( | |||
email, password | |||
email, password,name,bio,profile_picture |
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.
Nit: Let's move this to keyword arguments, rather than args... i.e.
self.services.auth.register(
email=email,
password=password,
…
)
py/core/main/api/v3/users_router.py
Outdated
profile_picture=profile_picture, | ||
) | ||
|
||
# if name or bio or profile_picture: |
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.
Feel free to delete this rather than commenting it out
py/core/providers/auth/r2r_auth.py
Outdated
@@ -381,6 +392,14 @@ async def change_password( | |||
id=user.id, | |||
new_hashed_password=hashed_new_password, | |||
) | |||
|
|||
try: | |||
await self.email_provider.send_password_changed_email( |
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.
Nit: keyword args rather than positional
py/core/providers/auth/r2r_auth.py
Outdated
user = await self.database_provider.users_handler.get_user_by_id( | ||
id=user_id | ||
) | ||
logger.info(user) |
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.
Not sure if it's necessary to keep this log, but if we do keep it let's make it more descriptive.
py/core/providers/email/sendgrid.py
Outdated
|
||
For security reasons, you will need to log in again on all your devices. | ||
""" | ||
html_body = """ |
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.
Not something that we need to worry about now, but as we add more email templates in the future it might be nice to move these to proper HTML files.
py/core/providers/email/smtp.py
Outdated
<h1>Password Changed Successfully</h1> | ||
<p>Your password has been successfully changed.</p> | ||
|
||
<p style="color: #d63939;"><strong>If you did not make this change, please contact support immediately and secure your account.</strong></p> |
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.
Also not something to worry about for now, but would be nice to make the colors for emails configurable in the future.
783bf4f
to
5d91432
Compare
4eb49e6
to
1cff015
Compare
🎯 Problem
Previously, setting user profile fields (name, bio, profile_picture) required two API calls:
This caused verification emails to use email prefix instead of actual name since the name was set after registration.
💡 Solution
Combined registration and profile update into a single API call by:
Important
Combine user registration and profile update into a single API call, ensuring immediate use of profile fields in verification emails.
users_router.py
.users_router.py
.r2r_auth.py
.create_user
inusers.py
to acceptname
,bio
, andprofile_picture
during registration.register
inr2r_auth.py
andsupabase.py
to handle new profile fields.register
inauth_service.py
to pass profile fields to providers.users_router.py
.This description was created by for 5d91432. It will automatically update as commits are pushed.