-
Notifications
You must be signed in to change notification settings - Fork 636
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 #664
base: master
Are you sure you want to change the base?
Solution #664
Conversation
class Meta: | ||
model = Actor | ||
fields = ("id", "first_name", "last_name", "full_name") | ||
|
||
def get_full_name(self, obj): |
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 also can wrote property for Actor method
cinema/serializers.py
Outdated
) | ||
|
||
def get_tickets_available(self, 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.
You also can calculate it in view
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.
This is inefficient because we will have to query database for tickets.count() on every entry in a list. Better to override queryset and use annotate()
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.
nice)
cinema/serializers.py
Outdated
for ticket_data in tickets_data: | ||
Ticket.objects.create(order=order, **ticket_data) | ||
|
||
return order |
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.
On create/update/delete functions you don't need to return anything
cinema/views.py
Outdated
@@ -31,28 +46,90 @@ class CinemaHallViewSet(viewsets.ModelViewSet): | |||
|
|||
|
|||
class MovieViewSet(viewsets.ModelViewSet): | |||
queryset = Movie.objects.all() | |||
queryset = Movie.objects.prefetch_related("genres", "actors").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.
You don't need to call all()
return super().get_serializer_class() | ||
|
||
def get_queryset(self): | ||
queryset = self.queryset |
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.
call super method instead of accesing this attribute directly
solve N+1 problem
cinema/views.py
Outdated
return super().get_serializer_class() | ||
|
||
def get_queryset(self): | ||
queryset = self.queryset.annotate( |
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 here
cinema/serializers.py
Outdated
) | ||
|
||
def get_tickets_available(self, 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.
This is inefficient because we will have to query database for tickets.count() on every entry in a list. Better to override queryset and use annotate()
cinema/serializers.py
Outdated
movie = MovieListSerializer(read_only=True) | ||
cinema_hall = CinemaHallSerializer(read_only=True) | ||
taken_places = serializers.SerializerMethodField() | ||
tickets_available = serializers.SerializerMethodField() |
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.
this should be shown only on List serializer
cinema/serializers.py
Outdated
class MovieSessionDetailSerializer(serializers.ModelSerializer): | ||
movie = MovieListSerializer(read_only=True) | ||
cinema_hall = CinemaHallSerializer(read_only=True) | ||
taken_places = serializers.SerializerMethodField() |
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 use nested serializer here instead - create TicketSeatsSerializer with row / seat fields and pass it to taken_places. then you will not need a separate method get_taken_places
cinema/serializers.py
Outdated
|
||
|
||
class OrderCreateSerializer(serializers.ModelSerializer): | ||
tickets = serializers.ListField() |
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.
instead create a modified version of ticket serializer without movie_session = MovieSessionListSerializer(read_only=True)
and pass it here
cinema/views.py
Outdated
return super().get_serializer_class() | ||
|
||
def get_queryset(self): | ||
queryset = super().get_queryset() |
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.
is better to access queryset like so : self.queryset
cinema/views.py
Outdated
def get_queryset(self): | ||
queryset = super().get_queryset() | ||
|
||
genres_param = self.request.query_params.get("genres") |
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.
all of this can go under if condition
cinema/views.py
Outdated
queryset = MovieSession.objects.all() | ||
queryset = ( | ||
MovieSession.objects.select_related | ||
("movie", "cinema_hall").prefetch_related("tickets").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.
you don't need to prefetch tickets on every action and you don't need all() in the end
cinema/views.py
Outdated
return queryset.distinct() | ||
|
||
|
||
class OrderPagination(PageNumberPagination): |
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.
would be better to put pagination in separate file
cinema/views.py
Outdated
Order.objects.prefetch_related | ||
("tickets__movie_session__movie", | ||
"tickets__movie_session__cinema_hall" | ||
).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.
no need for all() if you already used prefetch
cinema/views.py
Outdated
queryset = super().get_queryset().annotate( | ||
available_tickets=F("cinema_hall__rows") | ||
* F("cinema_hall__seats_in_row") - Count("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.
Why do you need to call it twice?
No description provided.