Develop #913
Develop #913
Conversation
There was a problem hiding this comment.
You've done a great job so far! 😊 However, there are a few areas that need some improvement. Please refer to the comments for more details. Keep up the good work! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
db/models.py
Outdated
| @@ -1,3 +1,5 @@ | |||
| from django.contrib.auth.models import User, AbstractUser | |||
There was a problem hiding this comment.
The User model is imported but not used in this file. It is a good practice to remove unused imports, as they can lead to confusion and make the code harder to read. Additionally, use get_user_model() instead of AbstractUser or User when you need to refer to the user model. This is a best practice in Django and allows for easier customization of the user model in the future.
db/models.py
Outdated
| created_at = models.DateTimeField(auto_now_add=True) | ||
| user = models.ForeignKey(to='User', | ||
| on_delete=models.CASCADE, | ||
| related_name="orders") |
There was a problem hiding this comment.
You're using a string to refer to the User model in the ForeignKey field. While this is not wrong and can be useful in avoiding circular import issues, it's better to use the settings.AUTH_USER_MODEL to refer to the user model. This allows for better flexibility if you decide to change the user model in the future.
db/models.py
Outdated
| class Order(models.Model): | ||
| created_at = models.DateTimeField(auto_now_add=True) | ||
| user = models.ForeignKey(to='User', | ||
| on_delete=models.CASCADE, | ||
| related_name="orders") | ||
|
|
||
| class Meta: | ||
| ordering = ("-created_at",) | ||
|
|
||
| def __str__(self) -> str: | ||
| return f"{self.created_at}" |
There was a problem hiding this comment.
The class Meta is implemented before some attributes are defined. Remember that it's a good practice to define all attributes before implementing class Meta.
| class User(AbstractUser): | ||
| pass |
There was a problem hiding this comment.
The User model is empty, it is defined but no fields are added. If you don't need to add any fields, you don't need to create this class. Django will use the default User model. If you want to add fields in the future, you can leave it, but it's better to add a comment explaining that.
| from django.db.models import QuerySet | ||
| from django.utils.datetime_safe import datetime | ||
|
|
||
| from db.models import Ticket, Order, User, MovieSession |
There was a problem hiding this comment.
According to the checklist, you should use get_user_model() instead of directly using the User model. This is a best practice in Django to avoid potential issues with custom user models.
| def create_order(tickets: list[Ticket], | ||
| username: str, | ||
| date: str = None) -> QuerySet: |
There was a problem hiding this comment.
The function create_order is missing a docstring. It's a good practice to include a docstring at the beginning of every function to describe its purpose, parameters, and return value.
|
|
||
|
|
||
| def get_orders(username: str = None) -> QuerySet: | ||
| if username: |
There was a problem hiding this comment.
The function get_orders is missing a docstring. It's a good practice to include a docstring at the beginning of every function to describe its purpose, parameters, and return value.
| from django.contrib.auth import get_user_model | ||
|
|
||
|
|
||
| User = get_user_model() |
There was a problem hiding this comment.
According to the checklist, you should use get_user_model() instead of User directly. You've imported the function but didn't use it in the rest of your code.
| user = User.objects.create_user(username=username) | ||
| user.set_password(password) | ||
| if email: | ||
| user.email = email | ||
| if first_name: | ||
| user.first_name = first_name | ||
| if last_name: | ||
| user.last_name = last_name | ||
|
|
||
| user.save() |
There was a problem hiding this comment.
The create_user method actually sets the password for the user, so there's no need to call set_password separately. You can pass the password as an argument to create_user. Also, you can pass other optional parameters like email, first_name, last_name directly to create_user if they are not None. This way, you can avoid multiple if checks and call save() only once.
| user = User.objects.get(pk=user_id) | ||
| if username: | ||
| user.username = username | ||
| if password: | ||
| user.set_password(password) | ||
| if email: | ||
| user.email = email | ||
| if first_name: | ||
| user.first_name = first_name | ||
| if last_name: | ||
| user.last_name = last_name | ||
|
|
||
| user.save() |
There was a problem hiding this comment.
In the update_user function, you can also reduce the number of if checks by using the update method of the QuerySet API. This method allows you to update certain fields of the model directly in the database, without having to load it into Python. This can be more efficient if you are updating a lot of instances at once because it only requires one database query. However, keep in mind that the update method does not call the save method or trigger the pre_save or post_save signals, so if you need those to happen, you should stick with your current approach.
There was a problem hiding this comment.
Great work! Your code meets the task requirements and follows the checklist. There are a few areas for improvement, but they don't significantly impact the functionality of your code. Keep going! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -42,3 +42,8 @@ def update_movie_session( | |||
|
|
|||
| def delete_movie_session_by_id(session_id: int) -> None: | |||
| MovieSession.objects.get(id=session_id).delete() | |||
There was a problem hiding this comment.
Same as the previous comment, it's recommended to handle exceptions where a record is not found in the database. You can use Django's get_object_or_404 function to raise a 404 error if the MovieSession object is not found.
| from django.db.models import QuerySet | ||
| from django.utils.datetime_safe import datetime | ||
|
|
||
| from db.models import Ticket, Order, User, MovieSession |
There was a problem hiding this comment.
The User model is imported directly. According to the checklist, you should use the get_user_model function for this purpose.
| def create_order(tickets: list[Ticket], | ||
| username: str, | ||
| date: str = None) -> QuerySet: | ||
| user = User.objects.get(username=username) |
There was a problem hiding this comment.
Here you are using User directly instead of get_user_model(). Replace User with get_user_model(). This is a best practice for Django.
| order = Order.objects.create(user=user) | ||
| if date: | ||
| order.created_at = datetime.strptime(date, "%Y-%m-%d %H:%M") | ||
| order.save(update_fields=["created_at"]) |
There was a problem hiding this comment.
The save() method is called twice for the Order object. According to the checklist, the save() method should be called only once if possible. You can set the created_at attribute when creating the Order object.
| def create_order(tickets: list[Ticket], | ||
| username: str, | ||
| date: str = None) -> QuerySet: |
There was a problem hiding this comment.
The date argument is optional, but there is no default value provided. This can lead to a TypeError if the date argument is not provided when the function is called. You should provide a default value for date.
| movie_session = (MovieSession.objects | ||
| .get(id=ticket_data["movie_session"])) |
There was a problem hiding this comment.
The get method is used here, which can raise a DoesNotExist exception if no MovieSession object with the provided id is found. You should handle this potential exception.
| first_name: str = None, | ||
| last_name: str = None, | ||
| ) -> User: | ||
| user = User.objects.create_user(username=username) |
There was a problem hiding this comment.
This line of code does not meet the requirement 'Don't Repeat Yourself' from the checklist. The save() method is called in the create_user method of the User model and you call it again on line 23. You can avoid this by creating the user instance without saving it to the database using User(username=username) and then save it after all changes on line 23.
| from django.contrib.auth import get_user_model | ||
|
|
||
|
|
||
| User = get_user_model() |
There was a problem hiding this comment.
According to the 'Code Style' section of the checklist, it is best to use get_user_model() instead of User. You've imported get_user_model, but then you've assigned it to User and used User throughout the code. It would be better to use get_user_model() directly when you need to access the User model.
| first_name: str = None, | ||
| last_name: str = None, | ||
| ) -> User: | ||
| user = User.objects.create_user(username=username) |
There was a problem hiding this comment.
This line of code does not meet the requirement 'Choose correct and suitable variable names' from the checklist. The variable user is suitable in this context, but remember to always use descriptive and meaningful variable names.
No description provided.