-
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' #663
base: master
Are you sure you want to change the base?
'Solution' #663
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.
Your task is not completed. You need to fix already existing code & provide filtering for movies and movie_sessions.
Good luck)
@@ -77,4 +87,52 @@ class MovieSessionDetailSerializer(MovieSessionSerializer): | |||
|
|||
class Meta: | |||
model = MovieSession | |||
fields = ("id", "show_time", "movie", "cinema_hall") | |||
fields = ("id", "show_time", "movie", "cinema_hall", "taken_places") |
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 added field taken_places, but model doesn't have this fields. You need to got it from tickets
@@ -68,6 +77,7 @@ class Meta: | |||
"movie_title", | |||
"cinema_hall_name", | |||
"cinema_hall_capacity", | |||
"tickets_available", |
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 need annotate method to your MovieSessionViewSet, to make this field available
|
||
from cinema.models import Genre, Actor, CinemaHall, Movie, MovieSession | ||
from cinema.models import Genre, Actor, CinemaHall, Movie, MovieSession, 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.
Spread imports on multiple lines
cinema/views.py
Outdated
def get_queryset(self): | ||
queryset = self.queryset.filter(user=self.request.user) | ||
if self.action == "list": | ||
queryset = queryset.prefetch_related("tickets__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.
It would be better to prefetch_related("tickets__movie_session__movie", "tickets__movie_session__cinema_hall") instead of tickets__movie_session. Because there are indepent models which have relations with model MovieSession
cinema/serializers.py
Outdated
class Meta: | ||
model = Ticket | ||
fields = [ | ||
"id", | ||
"order", | ||
"row", | ||
"seat", | ||
"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.
We don't need order field in this serializer
cinema/serializers.py
Outdated
class TicketSerializer(serializers.ModelSerializer): | ||
class Meta: | ||
model = Ticket | ||
fields = ("id", "row", "seat", "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.
we don't need id here
cinema/views.py
Outdated
class OrderSetPagination(PageNumberPagination): | ||
page_size = 1 | ||
page_size_query_param = "page_size" | ||
max_page_size = 3 |
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 would be better to create separated file for paginator. Do not mix ViewSets & Pagination in one class
title = request.query_params.get("title") | ||
|
||
if actors: | ||
actor_ids = actors.split(",") |
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 make sense to split and convert elements into int type - to be sure that you are being passed a valid numeric string + this should be placed in a separate method so you don't repeat yourself
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.
not done - you need to cast int type here.
cinema/models.py
Outdated
def clean(self): | ||
@staticmethod | ||
def ticket_validation( | ||
row: int, |
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.
overindented
cinema/models.py
Outdated
force_update=False, | ||
using=None, | ||
update_fields=None, | ||
self, |
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.
overindented - check everywhere
cinema/serializers.py
Outdated
|
||
|
||
class TicketPerformSerializer(serializers.ModelSerializer): | ||
movie_session = serializers.PrimaryKeyRelatedField( |
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 think this serializer in unnecessary here - we are just passing an id of movie session on ticket create. You will need movie_session nested serializer when you display a list of Orders instead
fields = ("id", "show_time", "movie", "cinema_hall", "taken_places") | ||
|
||
|
||
class TicketSerializer(serializers.ModelSerializer): |
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 inherit this from TicketPerformSerializer so you don't have to declare the same Meta class twice
|
||
return order | ||
|
||
def validate(self, attrs): |
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 validation logically belongs to Ticket serializer class
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.
not fixed
cinema/views.py
Outdated
if self.action == "list": | ||
queryset = ( | ||
queryset | ||
.select_related("cinema_hall", "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.
i guess here you will need to select_related for retrieve action as well
return queryset | ||
|
||
|
||
class OrderResultsSetPagination(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 make sense to break down logic and put pagination in separate file
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.
not done
cinema/views.py
Outdated
|
||
def get_queryset(self): | ||
query_set = self.queryset.select_related( | ||
"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.
why prefetch user? are you accessing any of user's fields?
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 will approve but please make sure you fix all the comments.
title = request.query_params.get("title") | ||
|
||
if actors: | ||
actor_ids = actors.split(",") |
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.
not done - you need to cast int type here.
|
||
return order | ||
|
||
def validate(self, attrs): |
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.
not fixed
return queryset | ||
|
||
|
||
class OrderResultsSetPagination(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.
not done
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 to make changes here
No description provided.