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

Develop #3

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Viacheslav417865
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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


Great job on your project! 🎉 Your implementation meets the task requirements effectively, and the core functionalities are well-covered. While there are a few minor issues and optimizations suggested, they do not critically impact the functionality of your code. Keep up the good work, and consider addressing the feedback to enhance your code quality further. Happy coding! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 task requirements! 🎉 The code meets the core functionality and constraints effectively. While there are some minor issues and suggestions for improvement, they do not significantly impact the functionality. Keep up the good work and consider the feedback for future enhancements! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

"""
try:
return jwt.decode(token, self._secret_key_access, algorithms=[self._algorithm])
except ExpiredSignatureError:
logger.warning(f"Access token expired: {token}") # Log the expiry
raise TokenExpiredError

Choose a reason for hiding this comment

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

The decode_access_token method raises a TokenExpiredError if the token is expired. Ensure that this exception is handled appropriately in your application.

@@ -68,9 +76,10 @@
try:
return jwt.decode(token, self._secret_key_refresh, algorithms=[self._algorithm])
except ExpiredSignatureError:
logger.warning(f"Refresh token expired: {token}")
raise TokenExpiredError

Choose a reason for hiding this comment

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

The decode_refresh_token method raises a TokenExpiredError if the token is expired. Ensure that this exception is handled appropriately in your application.

This will be a JWT token with a short expiration time.
"""
try:
expire = datetime.utcnow() + timedelta(minutes=JWTAuthManager._ACTIVATION_KEY_TIMEDELTA_MINUTES)

Choose a reason for hiding this comment

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

The create_activation_token function uses a fixed expiration time of 60 minutes. Ensure that this aligns with your application's requirements for activation tokens.

Create a password reset token for a user, with a short expiration time.
"""
try:
expire = datetime.utcnow() + timedelta(minutes=15)

Choose a reason for hiding this comment

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

The create_password_reset_token function uses a fixed expiration time of 15 minutes. Ensure that this aligns with your application's requirements for password reset tokens.

}

with patch("routes.accounts.Session.commit", side_effect=SQLAlchemyError):
with patch("src.routes.accounts.Session.commit", side_effect=SQLAlchemyError):

Choose a reason for hiding this comment

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

The patch decorator is used to simulate a database commit error. Ensure that the path to the Session.commit method is correct and that the test environment supports mocking.

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.

2 participants