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

solution #940

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

solution #940

wants to merge 4 commits into from

Conversation

spa7id
Copy link

@spa7id spa7id commented Nov 10, 2024

No description provided.

Copy link

@YaroslavBordovoy YaroslavBordovoy left a comment

Choose a reason for hiding this comment

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

In new models, specify related_name, it won't be superfluous.
Remove the changes in the tests, perhaps you accidentally made them.

services/order.py Outdated Show resolved Hide resolved
services/user.py Outdated Show resolved Hide resolved
services/order.py Outdated Show resolved Hide resolved
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.

Thank you for your hard work on this pull request! While you've done a great job addressing many aspects of the task, there are a few critical issues that need to be resolved before we can approve it:

  1. Exception Handling: There are multiple places where exception handling is necessary to prevent the application from crashing, such as in the update_user, get_orders, and delete_movie_session_by_id functions. Implementing proper exception handling will ensure the application handles errors gracefully.

  2. Return Types: Ensure that the return types in the function signatures match the actual return values, particularly in functions like get_orders and get_taken_seats.

  3. User Model Configuration: Double-check that the AUTH_USER_MODEL setting is correctly configured to use your custom User model if it is intended to replace the default one.

  4. Password Hashing: Verify that the create_user method hashes passwords correctly to avoid storing them in plain text.

These changes are crucial for the functionality and security of the application. Once you've addressed these issues, feel free to resubmit the pull request. Keep up the great work, and I'm confident you'll nail it! 😊

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

db/models.py Show resolved Hide resolved
services/movie_session.py Outdated Show resolved Hide resolved
services/movie_session.py Outdated Show resolved Hide resolved
services/order.py Outdated Show resolved Hide resolved
services/order.py Outdated Show resolved Hide resolved
services/order.py Outdated Show resolved Hide resolved
services/user.py Show resolved Hide resolved
services/user.py Show resolved Hide resolved
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 submission! 🎉 While the code is mostly well-structured and functional, there are a few critical areas that need attention to enhance security and robustness:

  1. Exception Handling:

    • Add exception handling for cases where a user or order with the given ID does not exist. This will prevent unhandled exceptions and improve user experience.
  2. Password Hashing:

    • Ensure that the create_user method hashes passwords correctly using the set_password method to avoid storing them in plain text.
  3. Type Annotations:

    • Verify and correct the return types for functions like get_orders and get_taken_seats to ensure clarity and compatibility, especially if using Python versions prior to 3.9.
  4. User Model Configuration:

    • Confirm that the AUTH_USER_MODEL setting is correctly configured to use your custom User model.

Addressing these issues will significantly improve the security and maintainability of your application. Keep up the good work, and continue refining your skills! 🚀

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

db/models.py Show resolved Hide resolved
services/movie.py Show resolved Hide resolved
services/movie_session.py Show resolved Hide resolved
services/order.py Show resolved Hide resolved
services/order.py Show resolved Hide resolved
services/user.py Show resolved Hide resolved
services/user.py Show resolved Hide resolved
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