Skip to content
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 #652

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Solution #652

wants to merge 8 commits into from

Conversation

AndriiHamasa
Copy link

No description provided.

Copy link

@dartomOOv dartomOOv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert all tests

Comment on lines 95 to 96
movie = MovieListSerializer(many=False, read_only=True)
cinema_hall = CinemaHallSerializer(many=False, read_only=True)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many by default is False

order = Order.objects.create(**validated_data)
for ticket_data in tickets_data:
Ticket.objects.create(order=order, **ticket_data)
return order

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In create/update/delete functions you don't need return anything

cinema/views.py Outdated
Comment on lines 56 to 59
genres_ids = [
int(genre_id.strip(" "))
for genre_id in genres.split(",")
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code duplicate, put in separate function

cinema/views.py Outdated
@@ -34,6 +38,29 @@ class MovieViewSet(viewsets.ModelViewSet):
queryset = Movie.objects.all()
serializer_class = MovieSerializer

def get_queryset(self):
queryset = self.queryset

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

cinema/views.py Outdated
@@ -48,6 +75,32 @@ class MovieSessionViewSet(viewsets.ModelViewSet):
queryset = MovieSession.objects.all()
serializer_class = MovieSessionSerializer

def get_queryset(self):
queryset = self.queryset

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

cinema/views.py Outdated
genres__id__in=self._params_to_int(genres)
)

return queryset

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call distinct here()

cinema/views.py Outdated
pagination_class = OrderPagination

def get_queryset(self):
queryset = self.queryset

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

cinema/views.py Outdated

def get_queryset(self):
queryset = super().get_queryset()
title = self.request.query_params.get("title")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would make sense only on list action

cinema/views.py Outdated
@@ -56,3 +112,32 @@ def get_serializer_class(self):
return MovieSessionDetailSerializer

return MovieSessionSerializer


class OrderPagination(LimitOffsetPagination):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is better design to put pagination in separate file

cinema/views.py Outdated
def get_queryset(self):
queryset = super().get_queryset()

date = self.request.query_params.get("date")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here also - this only applicable to list action

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not understandable for me, why in list we should call super, but in other cases no, or what?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriiHamasa Oleksandr meant that you should handle query params only when it's list action because query params are passed only for GET(for list) http method

cinema/views.py Outdated
@@ -34,6 +38,29 @@ class MovieViewSet(viewsets.ModelViewSet):
queryset = Movie.objects.all()
serializer_class = MovieSerializer

def get_queryset(self):
queryset = self.queryset
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

cinema/views.py Outdated
@@ -48,6 +75,32 @@ class MovieSessionViewSet(viewsets.ModelViewSet):
queryset = MovieSession.objects.all()
serializer_class = MovieSessionSerializer

def get_queryset(self):
queryset = self.queryset
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still not fixed

cinema/views.py Outdated
def get_queryset(self):
queryset = super().get_queryset()

date = self.request.query_params.get("date")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriiHamasa Oleksandr meant that you should handle query params only when it's list action because query params are passed only for GET(for list) http method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants