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

implement py-tickets-orders #650

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

Conversation

ValeriiaKyr
Copy link

No description provided.

Comment on lines 140 to 143
REST_FRAMEWORK = {
"DEFAULT_PAGINATION_CLASS":
"rest_framework.pagination.LimitOffsetPagination",
"PAGE_SIZE": 10

Choose a reason for hiding this comment

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

I recommend you to delete this and write your own custom paginator. It will be very easy, if you watched and read the lessons carefully ofc)
Moreover, try to use this new paginator ONLY with OrderViewSet. Because otherwise tests will fail :(

Comment on lines 123 to 126
class OrderSetPagination(PageNumberPagination):
page_size = 1
page_size_query_param = "page_size"
max_page_size = 10

Choose a reason for hiding this comment

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

I think it will be much better to replace this class into a separate module (paginators.py in the same directory ofc)

Copy link

@BastovOleksandr BastovOleksandr left a comment

Choose a reason for hiding this comment

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

Rejected only because your code doesn't pass tests

Comment on lines 4 to 7
class OrderSetPagination(PageNumberPagination):
page_size = 1
page_size_query_param = "page_size"
max_page_size = 10

Choose a reason for hiding this comment

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

page_size = 1
I think it's better to give at least 5

fields = ("id", "row", "seat", "movie_session")

def validate(self, attrs):
Ticket.validate_seat_and_row(

Choose a reason for hiding this comment

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

better call super() here and save the result, then after validation return result

Choose a reason for hiding this comment

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

This to be 100% sure that you return the same thing that is in the parent class

class MovieSessionDetailSerializer(MovieSessionSerializer):
movie = MovieListSerializer(many=False, read_only=True)
cinema_hall = CinemaHallSerializer(many=False, read_only=True)
taken_places = SerializerMethodField()

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 - create TicketSeatsSerializer with row / seat fields and pass it to taken_places

cinema/views.py Outdated
queryset = queryset.filter(title__icontains=title)

elif self.action == "retrieve":
queryset = queryset.prefetch_related("genres", "actors")

Choose a reason for hiding this comment

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

you already prefetch above - no need to do it twice

cinema/views.py Outdated
if date:
queryset = queryset.filter(show_time__date=date)

if self.action in "retrieve":

Choose a reason for hiding this comment

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

should be == Right now it will try to match self.action vs every character in a string

cinema/views.py Outdated
queryset = queryset.filter(title__icontains=title)

elif self.action == "retrieve":
queryset = queryset

Choose a reason for hiding this comment

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

what is this for?

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")

Choose a reason for hiding this comment

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

here would be better to specifically prefetch "tickets__movie_session__movie", "tickets__movie_session__cinema_hall"



class TicketRetrieveSerializer(TicketSerializer):
movie_session = MovieSessionListSerializer(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 is False be default

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 fucntion you don't need return anything

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

return 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.

call super method instead of accesing this attribute directly

Copy link

@Oleksl888 Oleksl888 left a comment

Choose a reason for hiding this comment

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

Good job!

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