-
Notifications
You must be signed in to change notification settings - Fork 857
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
solving #879
base: master
Are you sure you want to change the base?
solving #879
Conversation
db/models.py
Outdated
class Ticket(models.Model): | ||
movie_session = models.ForeignKey( | ||
to=MovieSession, | ||
on_delete=models.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.
related names missing here
db/models.py
Outdated
constraints = [ | ||
UniqueConstraint( | ||
fields=["movie_session", "row", "seat"], | ||
name="unique_movie_session" |
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.
constraint name should contain all fields that this contraint is applied to
db/models.py
Outdated
def __str__(self) -> str: | ||
return ( | ||
self.movie_session.movie.title + " " | ||
+ self.movie_session.show_time.strftime("%Y-%m-%d %H:%M:%S") |
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.
can you do it without string concatenation?
services/movie_session.py
Outdated
|
||
|
||
def get_taken_seats(movie_session_id: int) -> list: | ||
if MovieSession.objects.filter(id=movie_session_id).exists(): |
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.
no need for extra if's and prints, just return result - we don't need to try to catch all errors in this scenario
|
||
with transaction.atomic(): | ||
order = Order.objects.create(user=user) | ||
Order.objects.filter(id=order.id).update(**new_data) |
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.
or you could set created_at on order
object and then call save()
services/user.py
Outdated
last_name: str | None = None | ||
) -> None: | ||
if get_user_model().objects.filter(username=username).exists(): | ||
print("Username already exists") |
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.
no need for this check
services/user.py
Outdated
|
||
|
||
def get_user(user_id: int) -> User: | ||
return get_user_model().objects.filter(pk=user_id).first() |
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.
or you can use .get() here
services/user.py
Outdated
first_name: str | None = None, | ||
last_name: str | None = None | ||
) -> None: | ||
if not get_user_model().objects.filter(pk=user_id).exists(): |
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.
no need for this - if user not exists let the exception happen, and later when you work with django you may use get_object_or_404() method for cases like this. For now lets keep it simple
services/user.py
Outdated
print("User not found") | ||
return | ||
|
||
user = get_user_model().objects.get(pk=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 can reuse your get_user function here
services/user.py
Outdated
user.username != username | ||
and get_user_model().objects.filter(username=username).exists() | ||
): | ||
print("Username already exists") |
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.
no need for this checks really
on_delete=models.CASCADE, | ||
related_name="tickets" |
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.
did you push migrations that will update database?
No description provided.