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 #687

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

Solution #687

wants to merge 3 commits into from

Conversation

dpiuro
Copy link

@dpiuro dpiuro commented Sep 28, 2024

No description provided.

Copy link

@OleksiukStepan OleksiukStepan 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

if actors is not None:
instance.actors.set(actors)
if genres is not None:
instance.genres.set(genres)

Choose a reason for hiding this comment

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

You've implemented this logic in your Movie model. DRY (Don't Repeat Yourself)

class ActorList(generics.ListCreateAPIView):
queryset = Actor.objects.all()
serializer_class = ActorSerializer

Choose a reason for hiding this comment

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

You have to use GenericAPIView here. Check README.md
"·For the Actor model use a GenericAPIView"

cinema/views.py Outdated
if serializer.is_valid():
serializer.save()
return Response(serializer.data, status=status.HTTP_201_CREATED)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

Choose a reason for hiding this comment

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

You can simplify it using:
serializer.is_valid(raise_exception=True)
serializer.save()
return Response(serializer.data, status=status.HTTP_200_OK)

cinema/views.py Outdated
serializer = GenreSerializer(genres, many=True)
return Response(serializer.data)

def post(self, request):

Choose a reason for hiding this comment

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

Don't forget to add type hints to parameters and return

Comment on lines 1 to 2
from rest_framework import serializers
from cinema.models import Actor, Genre, CinemaHall, Movie
Copy link

Choose a reason for hiding this comment

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

split different groups of imports

Comment on lines 5 to 20
class ActorSerializer(serializers.ModelSerializer):
class Meta:
model = Actor
fields = ["id", "first_name", "last_name"]


class GenreSerializer(serializers.ModelSerializer):
class Meta:
model = Genre
fields = ["id", "name"]


class CinemaHallSerializer(serializers.ModelSerializer):
class Meta:
model = CinemaHall
fields = ["id", "name", "rows", "seats_in_row"]
Copy link

Choose a reason for hiding this comment

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

Yeah, it's a way easier to do it in this way, however we'll learn it in API Serializers topic, thus you have to implement it via a usual Serializer ;)

Comment on lines 28 to 35
actors = serializers.PrimaryKeyRelatedField(
many=True,
queryset=Actor.objects.all()
)
genres = serializers.PrimaryKeyRelatedField(
many=True,
queryset=Genre.objects.all()
)
Copy link

Choose a reason for hiding this comment

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

image

cinema/urls.py Outdated
Comment on lines 1 to 3
from django.urls import path, include
from rest_framework.routers import DefaultRouter
from cinema.views import (
Copy link

Choose a reason for hiding this comment

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

split different groups of imports

path("genres/<int:pk>/", GenreDetail.as_view(), name="genre-detail"),
path("actors/", ActorList.as_view(), name="actor-list"),
path("actors/<int:pk>/", ActorDetail.as_view(), name="actor-detail"),
path("", include(router.urls)),
Copy link

Choose a reason for hiding this comment

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

you have movies at the top of the list and now to connect router with "movies" url and viewset. Don't you think it'll conflict?

cinema/views.py Outdated
Comment on lines 1 to 7
from rest_framework.decorators import api_view
from rest_framework.response import Response
from rest_framework import status

from rest_framework.views import APIView
from rest_framework import status, generics, viewsets, mixins
from django.shortcuts import get_object_or_404
from rest_framework.request import Request
from typing import Any
Copy link

Choose a reason for hiding this comment

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

reorder imports + split different groups

@dpiuro dpiuro requested a review from MNDonut October 2, 2024 13:59
Copy link

@LiudmylaKulinchenko LiudmylaKulinchenko 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.

4 participants