-
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
Solution #912
base: master
Are you sure you want to change the base?
Solution #912
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.
Thanks for your hard work! There are a few changes that need to be made before we can move forward. Please address the security concern related to the SECRET_KEY exposure and consider the code quality issues mentioned, such as unnecessary save
method calls and direct User
model import. Keep going, you're doing great! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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 overall! 🎉 There are a few minor issues related to code style and best practices, but they don't significantly impact the functionality of your code. Keep up the good work and always strive to improve your code quality! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
|
||
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.
Consider using get_user_model()
method instead of directly referring to the User
model. This is a good practice in Django as it allows for flexibility in case you decide to use a custom user model in the future.
return queryset | ||
|
||
|
||
def get_movie_by_id(movie_id: int) -> Movie: | ||
return Movie.objects.get(id=movie_id) | ||
|
||
|
||
@transaction.atomic | ||
def create_movie( | ||
movie_title: str, |
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.
According to the code style guide, you should use double quotes instead of single quotes. It would be better to use double quotes for string values to keep the code style consistent.
return queryset | ||
|
||
|
||
def get_movie_by_id(movie_id: int) -> Movie: | ||
return Movie.objects.get(id=movie_id) | ||
|
||
|
||
@transaction.atomic | ||
def create_movie( | ||
movie_title: str, | ||
movie_description: str, |
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 comment applies here as well, please use double quotes for string values.
@@ -42,3 +41,12 @@ 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.
Here you are using get()
method which will raise a DoesNotExist
exception if no object found. It would be better to use filter()
method along with delete()
which will not raise an exception if no object found.
|
||
|
||
def get_taken_seats(movie_session_id: int) -> list[dict]: | ||
movie_session = get_movie_session_by_id(movie_session_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.
As per the checklist, it is recommended to use get_user_model()
instead of User
. Although you have not used User
here, it's important to keep this in mind in other parts of your code as well.
username: str, | ||
date: str = None | ||
) -> QuerySet: | ||
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.
Code Style: You are using single quotes in get(username=username)
. It's better to use double quotes in the whole project for consistency. Replace it with get(username=username)
.
row=ticket["row"], | ||
seat=ticket["seat"] | ||
) | ||
order.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.
Don't Repeat Yourself: You are calling the save()
method even if the date
is not provided. It's better to call save()
only if the date
is provided, to avoid unnecessary database operations. You can use the update_fields
argument of the save()
method to save only the created_at
field. Like this:
python
if date:
order.created_at = date
order.save(update_fields=['created_at'])
|
||
|
||
def get_orders(username: str = None) -> QuerySet: | ||
orders = Order.objects.all() |
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: You are using single quotes in Order.objects.all()
. It's better to use double quotes in the whole project for consistency. Replace it with Order.objects.all()
.
def get_orders(username: str = None) -> QuerySet: | ||
orders = Order.objects.all() | ||
if username: | ||
return orders.filter(user__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: You are using single quotes in filter(user__username=username)
. It's better to use double quotes in the whole project for consistency. Replace it with filter(user__username=username)
.
No description provided.