-
Notifications
You must be signed in to change notification settings - Fork 48
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
Develop #24
base: main
Are you sure you want to change the base?
Develop #24
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.
Great job on implementing the user account management endpoints! 🎉 Your code is well-structured and meets the task requirements. While there are a few areas for improvement, such as handling password hashing, token validation, and using the Config
class for Pydantic models, these are minor and don't impact the core functionality. Keep up the good work and consider these suggestions for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
) | ||
|
||
try: | ||
user.password = data.password |
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.
Ensure that the password is hashed before being stored. If UserModel
does not handle hashing internally, consider using a hashing function here.
token_record = db.query(PasswordResetTokenModel).filter_by(user_id=user.id).first() | ||
|
||
expires_at = cast(datetime, token_record.expires_at).replace(tzinfo=timezone.utc) | ||
|
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.
Check if token_record
is not None
before accessing token_record.expires_at
to avoid AttributeError
.
""" | ||
try: | ||
decoded_token = jwt_manager.decode_refresh_token(token_data.refresh_token) | ||
user_id = decoded_token.get("user_id") |
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.
Ensure that user_id
is not None
after decoding the token. Handle the case where decoded_token
might not contain user_id
.
@field_validator("password") | ||
@classmethod | ||
def validate_password(cls, value): | ||
return accounts_validators.validate_password_strength(value) |
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.
Ensure that accounts_validators.validate_password_strength
is correctly implemented and imported. This function should enforce the desired password strength requirements.
model_config = { | ||
"from_attributes": True | ||
} |
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 model_config
attribute is not a standard Pydantic feature. Consider using a Config
class for model configuration, e.g., class Config: from_attributes = True
.
model_config = { | ||
"from_attributes": True | ||
} |
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.
Similar to the previous comment, consider using a Config
class for model configuration instead of model_config
.
No description provided.