Finished #773
Finished #773
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While you've made a great effort, there are a few critical issues that need to be addressed before we can approve the pull request.
-
Incorrect Serializer Usage: In both the
GenreListandGenreDetailclasses, theMovieSerializeris used instead of theGenreSerializer. This needs to be corrected to ensure that the correct data is being serialized. -
Incorrect Model Reference: The
get_objectmethod inGenreDetailis incorrectly referencing theMoviemodel instead of theGenremodel. This should be updated to useget_object_or_404(Genre, pk=pk). -
CinemaHallViewSet Serializer Class: The
CinemaHallViewSetis incorrectly set withCinemaHall.objects.all()as theserializer_class. It should be set toCinemaHallSerializer. -
Create Method Issues: The
createmethod inCinemaSerializeris incorrectly usingMovie.objects.create, and thecreatemethod inGenreSerializeris incorrectly usingActor.objects.create. These should be corrected to useCinemaHall.objects.createandGenre.objects.create, respectively.
These issues are critical as they impact the core functionality of the application. Once these are resolved, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need any help. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
cinema/serializers.py
Outdated
| name = serializers.CharField(max_length=255) | ||
|
|
||
| def create(self, validated_data): | ||
| return Actor.objects.create(**validated_data) |
There was a problem hiding this comment.
The create method in GenreSerializer is incorrectly using Actor.objects.create. It should use Genre.objects.create to match the model being serialized.
cinema/serializers.py
Outdated
| seats_in_row = serializers.IntegerField() | ||
|
|
||
| def create(self, validated_data): | ||
| return Movie.objects.create(**validated_data) |
There was a problem hiding this comment.
The create method in CinemaSerializer is incorrectly using Movie.objects.create. It should use CinemaHall.objects.create to match the model being serialized.
cinema/urls.py
Outdated
| path("movies/<int:pk>/", movie_detail, name="movie-detail"), | ||
| path("", include(router.urls)), | ||
| path("genres/", GenreList.as_view(), name="genre-list"), | ||
| path("genres/<int:pk>", GenreDetail.as_view(), name="genre-detail"), |
There was a problem hiding this comment.
Consider adding a trailing slash to the genres/<int:pk> path for consistency with the other URL patterns.
cinema/urls.py
Outdated
| path("genres/", GenreList.as_view(), name="genre-list"), | ||
| 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"), |
There was a problem hiding this comment.
Consider adding a trailing slash to the actors/<int:pk> path for consistency with the other URL patterns.
cinema/views.py
Outdated
|
|
||
| def get(self, request): | ||
| genres = Genre.objects.all() | ||
| serializer = MovieSerializer(genres, many=True) |
There was a problem hiding this comment.
The GenreList class is using MovieSerializer instead of GenreSerializer for the get method. This should be corrected to GenreSerializer to match the model being serialized.
cinema/views.py
Outdated
| class GenreDetail(APIView): | ||
|
|
||
| def get_object(self, pk): | ||
| return get_object_or_404(Movie, pk=pk) |
There was a problem hiding this comment.
The get_object method in GenreDetail is incorrectly using Movie instead of Genre. It should be get_object_or_404(Genre, pk=pk) to match the context of the class.
cinema/views.py
Outdated
| serializer = MovieSerializer(movie) | ||
| def get(self, request, pk): | ||
| genre = self.get_object(pk) | ||
| serializer = MovieSerializer(genre) |
There was a problem hiding this comment.
The GenreDetail class is using MovieSerializer instead of GenreSerializer for the get method. This should be corrected to GenreSerializer to match the model being serialized.
cinema/views.py
Outdated
| viewsets.GenericViewSet | ||
| ): | ||
| queryset = CinemaHall.objects.all() | ||
| serializer_class = CinemaHall.objects.all() |
There was a problem hiding this comment.
The CinemaHallViewSet is incorrectly assigning CinemaHall.objects.all() to serializer_class. It should be CinemaHallSerializer to correctly serialize the CinemaHall model.
There was a problem hiding this comment.
Great job on addressing the previous issues and refining your code! 🎉 Your migration file, models, and views are well-structured and meet the task requirements. However, there are a few areas to improve:
- Redundancy in
MovieSerializer: You've duplicated the lines updatingtitleanddescription. Please remove these duplicates to enhance code quality and avoid redundancy.
Overall, your solution is solid and meets the task requirements. The decision to approve is to help you move forward with your learning journey. Please review the comments and address the mentioned issues to improve your code further. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| instance.title = validated_data.get("title", instance.title) | ||
| instance.description = validated_data.get("description", | ||
| instance.description) |
There was a problem hiding this comment.
The lines updating title and description are duplicated. Remove these lines to avoid redundancy, as they are already updated in lines 16-19.
No description provided.