Solution#1163
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has two critical issues that need to be fixed before approval. First, the MovieSerializer.update() method in serializers.py doesn't handle actors and genres fields (many-to-many relationships), which means PUT/PATCH requests on movies will fail to update these relationships—this is a functional bug that breaks the API. Second, there are checklist item #2 violations: ActorList uses CreateModelMixin which internally raises exceptions instead of returning serializer.errors in a Response, and ActorDetail uses UpdateModelMixin with the same issue. Both views should manually handle validation like the GenreList.post method does, returning Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) instead of relying on mixin exception behavior. Please fix the MovieSerializer.update() method to properly extract and set actors and genres from validated_data, and update the Actor views to explicitly return validation errors in a Response.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| ), | ||
| migrations.CreateModel( | ||
| name='Genre', | ||
| fields=[ | ||
| ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('name', models.CharField(max_length=256, unique=True)), | ||
| ], | ||
| ), | ||
| migrations.CreateModel( | ||
| name='Movie', | ||
| fields=[ |
There was a problem hiding this comment.
The MovieSerializer.update method doesn't handle actors and genres fields. When updating a movie via PUT or PATCH, these many-to-many fields won't be updated. Consider extracting and setting them similar to the create method.
|
|
||
| class ActorList(GenericAPIView, | ||
| ListModelMixin, | ||
| CreateModelMixin): | ||
| queryset = Actor.objects.prefetch_related("movies") | ||
| serializer_class = ActorSerializer | ||
|
|
||
| def get(self, request, *args, **kwargs): | ||
| return self.list(request, *args, **kwargs) | ||
|
|
||
| def post(self, request, *args, **kwargs): | ||
| return self.create(request, *args, **kwargs) |
There was a problem hiding this comment.
Checklist item #2 violation: ActorList uses CreateModelMixin which internally calls is_valid(raise_exception=True), not returning serializer.errors in a Response. Consider manually handling validation similar to GenreList.post to return Response(serializers.errors, status=status.HTTP_400_BAD_REQUEST).
|
|
||
| class ActorDetail(GenericAPIView, | ||
| RetrieveModelMixin, | ||
| UpdateModelMixin, | ||
| DestroyModelMixin): | ||
| queryset = Actor.objects.prefetch_related("movies") | ||
| serializer_class = ActorSerializer | ||
|
|
||
| def get(self, request, *args, **kwargs): | ||
| return self.retrieve(request, *args, **kwargs) | ||
|
|
||
| def put(self, request, *args, **kwargs): | ||
| return self.update(request, *args, **kwargs) | ||
|
|
||
| def patch(self, request, *args, **kwargs): | ||
| return self.partial_update(request, *args, **kwargs) | ||
|
|
||
| def delete(self, request, *args, **kwargs): | ||
| return self.destroy(request, *args, **kwargs) |
There was a problem hiding this comment.
Checklist item #2 violation: ActorDetail uses UpdateModelMixin which internally handles validation by raising exceptions, not returning serializer.errors. Consider manually handling validation to return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The Actor views (ActorList and ActorDetail) in cinema/views.py still violate checklist item #2 - they use mixin methods (CreateModelMixin.create(), UpdateModelMixin.update()/partial_update()) which raise ValidationError exceptions instead of returning Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST). You must override post() in ActorList and both put() and patch() in ActorDetail to manually check is_valid() and return error responses, exactly like your GenreList.post(), GenreDetail.put(), and GenreDetail.patch() methods do (lines 33-36 and 52-63). Here's the pattern from GenreList:
def post(self, request, *args, **kwargs):
serializer = self.get_serializer(data=request.data)
if not serializer.is_valid():
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
self.perform_create(serializer)
return Response(serializer.data, status=status.HTTP_201_CREATED)Apply the same pattern to ActorList and ActorDetail - this is the only remaining blocker preventing approval.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| "last_name", | ||
| instance.last_name) | ||
| instance.save() | ||
| return instance | ||
|
|
||
|
|
||
| class CinemaHallSerializer(serializers.Serializer): | ||
| id = serializers.IntegerField(read_only=True) | ||
| name = serializers.CharField(max_length=64) | ||
| rows = serializers.IntegerField() | ||
| seats_in_row = serializers.IntegerField() | ||
|
|
||
| def create(self, validated_data): |
There was a problem hiding this comment.
This violates checklist item #2: CreateModelMixin.create() internally raises exceptions when validation fails instead of returning Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST). The post method should manually validate and return errors like GenreList.post does.
|
|
||
| def post(self, request, *args, **kwargs): |
There was a problem hiding this comment.
This violates checklist item #2: The post method calls self.create() from CreateModelMixin, which raises ValidationError on invalid data instead of returning Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST). Unlike GenreList.post() (lines 33-36) which manually checks is_valid() and returns errors, this relies on mixin exception behavior.
|
|
||
| def put(self, request, *args, **kwargs): |
There was a problem hiding this comment.
This violates checklist item #2: The put method calls self.update() from UpdateModelMixin, which raises ValidationError on invalid data instead of returning Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST). Unlike GenreDetail.put() (lines 52-55) which manually checks is_valid() and returns errors.
|
|
||
| def patch(self, request, *args, **kwargs): |
There was a problem hiding this comment.
This violates checklist item #2: The patch method calls self.partial_update() from UpdateModelMixin, which raises ValidationError on invalid data instead of returning Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST). Unlike GenreDetail.patch() (lines 60-63) which manually checks is_valid() and returns errors.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved with an educational override to help you progress while learning. All core requirements are met - all four models are correctly implemented with proper fields, related_name='movies' is defined for ManyToManyFields, and your views properly return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) on validation failures [CHECKLIST ITEM #2]. Imports are correctly grouped using () [CHECKLIST ITEM #3]. There's a minor optimization opportunity in ActorDetail: you're using Actor.objects.get(pk=...) which performs a redundant database query when GenericAPIView already provides self.get_object() for this purpose. This is a common pattern in Django REST Framework - using the built-in methods helps keep your code clean and efficient. Great job on implementing all CRUD endpoints with the correct view classes!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| "seats_in_row", instance.seats_in_row) | ||
| instance.save() | ||
| return instance |
There was a problem hiding this comment.
Use self.get_object() instead of Actor.objects.get(pk=...) to avoid redundant database query. GenericAPIView already provides this method.
2 participants
No description provided.