-
Notifications
You must be signed in to change notification settings - Fork 859
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 #906
base: master
Are you sure you want to change the base?
Develop #906
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 work so far! However, there are a few issues that need to be addressed. Please ensure to only call save()
if any of the fields were updated and consider using get_user_model()
instead of directly referring to the User
model. Keep it up! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
class Order(models.Model): | ||
created_at = models.DateTimeField(auto_now_add=True) | ||
user = models.ForeignKey(on_delete=CASCADE, to="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.
The User
model is referenced before it is defined. This can lead to confusion and potential errors. It is a good practice to define all models before their usage. Consider moving the User
model definition before the Order
model.
db/models.py
Outdated
if self.row < 1 or self.row > 10: | ||
raise ValidationError({ | ||
"row": | ||
"row number must be in available range: (1, rows): (1, 10)" | ||
}) | ||
if self.seat < 1 or self.seat > 12: | ||
raise ValidationError({ | ||
"seat": ( | ||
"seat number must be in available range: " | ||
"(1, seats_in_row): (1, 12)" | ||
) | ||
}) |
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 number of rows and seats are hard-coded in the Ticket
model. This is not a good practice as the number of rows and seats can vary from one cinema hall to another. Consider fetching the number of rows and seats from the associated CinemaHall
instance.
db/models.py
Outdated
class User(AbstractUser): | ||
pass |
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 User
model is not extended. While this is not necessarily an error, it might limit the flexibility of your application. Consider adding extra fields that might be needed in your application, like date_of_birth
, profile_picture
, etc.
services/order.py
Outdated
@@ -0,0 +1,32 @@ | |||
from db.models import 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.
Code Style: Use get_user_model()
instead of User
, it is the best practice. Replace the import of User
with get_user_model
from django.contrib.auth
and use it to get the user.
services/order.py
Outdated
username: str, | ||
date: str = None | ||
) -> Order: | ||
user = User.objects.get(username=username) |
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.
Code Style: Use get_user_model()
instead of User
, it is the best practice. Replace User
with the call to get_user_model()
.
services/user.py
Outdated
@@ -0,0 +1,48 @@ | |||
from db.models import 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.
You're importing the User model directly, which is not a best practice. You should use get_user_model()
from django.contrib.auth
to get the user model.
services/user.py
Outdated
user = User.objects.create_user(username=username, | ||
password=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.
You're calling User.objects.create_user()
with just the username
and password
parameters. This method can also accept email
, first_name
, and last_name
as parameters. You can pass these parameters directly instead of setting them later. This way you can avoid calling the save()
method multiple times, which is against the DRY (Don't Repeat Yourself) principle.
user.first_name = first_name | ||
if last_name: | ||
user.last_name = last_name | ||
user.save() |
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.
You're saving the user object even if no changes were made to it. You should only call save()
if any of the fields were updated.
services/user.py
Outdated
first_name: str = None, | ||
last_name: str = None) -> User: | ||
|
||
user = User.objects.get(id=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.
You're using User.objects.get()
to retrieve the user object, which will raise a User.DoesNotExist
exception if the user with the given ID doesn't exist. It would be better to use get_object_or_404()
function, which handles this exception and raises a HTTP 404 error if the object doesn't exist.
if last_name: | ||
user.last_name = last_name | ||
|
||
user.save() |
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.
Same as before, you're saving the user object even if no changes were made to it. You should only call save()
if any of the fields were updated.
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.
Good job on your work so far! 👏 However, there's a small but important issue that needs to be addressed. In your create_user
and update_user
functions, you're calling save()
even when no changes were made to the user. This can impact the performance of your code. Try to update your code so that save()
is only called if at least one field was updated. Keep up the good work and happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
username: str, | ||
date: str = None | ||
) -> Order: | ||
user = get_user_model().objects.get(username=username) |
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.
It's recommended to use get_user_model()
instead of User
in your code. This is a best practice that ensures your code will always use the correct user model, even if it's been overridden in your project's settings.
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.
I've used get_user_model() here
services/order.py
Outdated
order = Order.objects.create(user=user,) | ||
if date: | ||
order.created_at = date | ||
|
||
for ticket in tickets: | ||
Ticket.objects.create(movie_session_id=ticket["movie_session"], | ||
order=order, | ||
row=ticket["row"], | ||
seat=ticket["seat"] | ||
) | ||
order.save(update_fields=["created_at"] if date else None) |
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.
You are saving the order
instance multiple times which is against the DRY principle. You should call the save()
method only once if it's possible. Try to organize your code in such a way that you only need to call save()
once, after all changes to the instance have been made.
services/order.py
Outdated
) -> Order: | ||
user = get_user_model().objects.get(username=username) | ||
with transaction.atomic(): | ||
order = Order.objects.create(user=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.
You are using single quotes in your code. It's better to stick to one style of quotes in your code. Double quotes are preferable.
last_name: str = None) -> None: | ||
user_model = get_user_model() | ||
user = user_model(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() | ||
return 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.
In the create_user
function, you're calling user.save()
unconditionally. This means that even if no changes were made to the user, the save()
method will still be called. This is unnecessary and can degrade the performance of your application. Instead, you should only call save()
if at least one field was updated. You can achieve this by adding a boolean variable that tracks whether any field was updated. If this variable is True
at the end of the function, call save()
. Otherwise, don't call it.
def update_user(user_id: int, | ||
username: str = None, | ||
password: str = None, | ||
email: str = None, | ||
first_name: str = None, | ||
last_name: str = None) -> None: | ||
|
||
user = get_user_model().objects.get(id=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() | ||
return 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.
The same issue is present in the update_user
function. You're calling user.save()
even if no changes were made to the user. To improve the performance of your code, only call save()
if at least one field was updated. You can use a boolean variable to track whether any field was updated. If this variable is True
at the end of the function, call save()
. Otherwise, don't call it.
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 work on this Pull Request! Your code follows Django's migrations best practices and is generally well-written. However, there are a few areas that require your attention for improvement. Please review the comments regarding the handling of cases when the MovieSession
with the provided id does not exist in the get_movie_session_by_id
and delete_movie_session_by_id
functions. Also, consider optimizing your update_movie_session
function to call save()
only when necessary. Lastly, remember to use get_user_model()
instead of User
. Keep up the good work and continue learning! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
class Order(models.Model): | ||
created_at = models.DateTimeField(auto_now_add=True) | ||
user = models.ForeignKey(on_delete=CASCADE, to="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.
Code Style: It's better to use get_user_model()
instead of User
, it's a best practice. You can import it from django.contrib.auth
.
|
||
|
||
class Ticket(models.Model): | ||
movie_session = models.ForeignKey(to=MovieSession, on_delete=CASCADE) |
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.
Code Style: It's better to use get_user_model()
instead of User
, it's a best practice. You can import it from django.contrib.auth
.
@@ -28,13 +33,14 @@ def create_movie( | |||
genres_ids: list = None, |
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.
Code Style: It's a good practice to clearly define the type of elements in the list. Instead of list
you can use list[int]
to specify that the list contains integers.
@@ -28,13 +33,14 @@ | |||
genres_ids: list = None, | |||
actors_ids: list = None, |
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.
Code Style: It's a good practice to clearly define the type of elements in the list. Instead of list
you can use list[int]
to specify that the list contains integers.
if genres_ids: | ||
movie.genres.set(genres_ids) | ||
if actors_ids: | ||
movie.actors.set(actors_ids) | ||
|
||
return movie |
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.
Don't Repeat Yourself: It's better to call movie.save()
once after all changes instead of calling set()
method for each field separately. This way you can reduce the number of database queries.
@@ -42,3 +42,13 @@ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issue with the delete_movie_session_by_id
function - it does not handle the case when the MovieSession
with the provided id does not exist. It's a bug.
No description provided.